Summary:
We used to carry patches for Tokio 0.2 to add support for disabling Tokio coop
(which was necessary to make Mononoke work with it), but this was upstreamed
in Tokio 1.x (as a different implementation), so that's no longer needed. Nobody
else besides Mononoke was using this.
For Hyper we used to carry a patch with a bugfix. This was also fixed in Tokio
1.x-compatible versions of Hyper. There are still users of hyper-02 in fbcode.
However, this is only used for servers and only when accepting websocket
connections, and those users are just using Hyper as a HTTP client.
Reviewed By: farnz
Differential Revision: D28091331
fbshipit-source-id: de13b2452b654be6f3fa829404385e80a85c4420
Summary:
This used to be used by Mononoke, but we're now on Tokio 1.x and on
corresponding versions of Gotham so it's not needed anymore.
Reviewed By: farnz
Differential Revision: D28091091
fbshipit-source-id: a58bcb4ba52f3f5d2eeb77b68ee4055d80fbfce2
Summary:
The changesets object is only valid to access the changesets of a single repo
(other repos may have different metadata database config), so it is pointless
for all methods to require the caller to provide the correct one. Instead,
make the changesets object remember the repo id.
Reviewed By: krallin
Differential Revision: D27430611
fbshipit-source-id: bf2c398af2e5eb77c1c7c55a89752753020939ab
Summary:
NOTE: there is one final pre-requisite here, which is that we should default all Mononoke binaries to `--use-mysql-client` because the other SQL client implementations will break once this lands. That said, this is probably the right time to start reviewing.
There's a lot going on here, but Tokio updates being what they are, it has to happen as just one diff (though I did try to minimize churn by modernizing a bunch of stuff in earlier diffs).
Here's a detailed list of what is going on:
- I had to add a number `cargo_toml_dir` for binaries in `eden/mononoke/TARGETS`, because we have to use 2 versions of Bytes concurrently at this time, and the two cannot co-exist in the same Cargo workspace.
- Lots of little Tokio changes:
- Stream abstractions moving to `tokio-stream`
- `tokio::time::delay_for` became `tokio::time::sleep`
- `tokio::sync:⌚:Sender::send` became `tokio::sync:⌚:Sender::broadcast`
- `tokio::sync::Semaphore::acquire` returns a `Result` now.
- `tokio::runtime::Runtime::block_on` no longer takes a `&mut self` (just a `&self`).
- `Notify` grew a few more methods with different semantics. We only use this in tests, I used what seemed logical given the use case.
- Runtime builders have changed quite a bit:
- My `no_coop` patch is gone in Tokio 1.x, but it has a new `tokio::task::unconstrained` wrapper (also from me), which I included on `MononokeApi::new`.
- Tokio now detects your logical CPUs, not physical CPUs, so we no longer need to use `num_cpus::get()` to figure it out.
- Tokio 1.x now uses Bytes 1.x:
- At the edges (i.e. streams returned to Hyper or emitted by RepoClient), we need to return Bytes 1.x. However, internally we still use Bytes 0.5 in some places (notably: Filestore).
- In LFS, this means we make a copy. We used to do that a while ago anyway (in the other direction) and it was never a meaningful CPU cost, so I think this is fine.
- In Mononoke Server it doesn't really matter because that still generates ... Bytes 0.1 anyway so there was a copy before from 0.1 to 0.5 and it's from 0.1 to 1.x.
- In the very few places where we read stuff using Tokio from the outside world (historical import tools for LFS), we copy.
- tokio-tls changed a lot, they removed all the convenience methods around connecting. This resulted in updates to:
- How we listen in Mononoke Server & LFS
- How we connect in hgcli.
- Note: all this stuff has test coverage.
- The child process API changed a little bit. We used to have a ChildWrapper around the hg sync job to make a Tokio 0.2.x child look more like a Tokio 1.x Child, so now we can just remove this.
- Hyper changed their Websocket upgrade mechanism (you now need the whole `Request` to upgrade, whereas before that you needed just the `Body`, so I changed up our code a little bit in Mononoke's HTTP acceptor to defer splitting up the `Request` into parts until after we know whether we plan to upgrade it.
- I removed the MySQL tests that didn't use mysql client, because we're leaving that behind and don't intend to support it on Tokio 1.x.
Reviewed By: mitrandir77
Differential Revision: D26669620
fbshipit-source-id: acb6aff92e7f70a7a43f32cf758f252f330e60c9
Summary:
Collecting into SortedVecMap an unsorted iterator is inefficient, because of
how [try_collect()](https://docs.rs/futures-util/0.3.0/src/futures_util/stream/try_stream/try_collect.rs.html#57) works. It calls `extend()` method every time a new element was fetched from the
stream. So if stream was unsorted, then we ran into a quadratic behaviour with
lots of reallocations.
Let's instead collect into BTreeMap, and then convert it into SortedVecMap.
Reviewed By: markbt
Differential Revision: D27997469
fbshipit-source-id: 58f837e6cc946ccc8809cce3d7a5a2e6ca24df40
Summary:
Update the zstd crates.
This also patches async-compression crate to point at my fork until upstream PR https://github.com/Nemo157/async-compression/pull/117 to update to zstd 1.4.9 can land.
Reviewed By: jsgf, dtolnay
Differential Revision: D27942174
fbshipit-source-id: 26e604d71417e6910a02ec27142c3a16ea516c2b
Summary: Enables the `Serialize` and `Deserialize` impls on the `Uuid` type.
Reviewed By: dtolnay
Differential Revision: D27799952
fbshipit-source-id: 4b0e2f8ab4ede20a2113fc1dda42c2ba8b3d0b35
Summary:
In the next diff it's going to be used to copy bonsais from the prod repo
during hg sync job, and in this diff I move this code to the common place so
that we can use it in the next diff
Differential Revision: D27852340
fbshipit-source-id: 9744571430e15a9d7f1e569d9b6690bc45787bd2
Summary:
Convert `BlobRepo` to a `facet::container`. This will allow it to be built
from an appropriate facet factory.
This only changes the definition of the structure: we still use
`blobrepo_factory` to construct it. The main difference is in the types
of the attributes, which change from `Arc<dyn Trait>` to
`Arc<dyn Trait + Send + Sync + 'static`>, specified by the `ArcTrait` alias
generated by the `#[facet::facet]` macro.
Reviewed By: StanislavGlebik
Differential Revision: D27169437
fbshipit-source-id: 3496b6ee2f0d1e72a36c9e9eb9bd3d0bb7beba8b
Summary:
In preparation for making `BlobRepo` buildable by facet factories, restore
`BlobRepo` members that had been converted to `TypeMap` attributes back into
real members.
This re-introduces some dependencies that were previously removed, but this
will be cleaned up when crates no longer have to depend on BlobRepo directly,
just the traits they are interested in.
Reviewed By: ahornby
Differential Revision: D27169422
fbshipit-source-id: 14354e6d984dfdd2be5c169f527e5f998f00db1e
Summary:
BonsaiChangesets are rarely mutated, and their maps are stored in sorted order,
so we can use `SortedVectorMap` to load them more efficiently.
In the cases where mutable maps of filechanges are needed, we can use `BTreeMap`
during the mutation and then convert them to `SortedVectorMap` to store them.
Reviewed By: mitrandir77
Differential Revision: D25615279
fbshipit-source-id: 796219c1130df5cb025952bb61002e8d2ae898f4
Summary:
Remove case conflict checking on upload. Disallowing case conflicts will
always be done during bookmark movement by checking the skeleton manifests.
A side-effect of this change is that configuring a repository with case
conflict checks, but not enabling skeleton manifests, will mean that commits
can't be landed in that repository, as there are no skeleton manifests to
check.
Reviewed By: ahornby
Differential Revision: D26781269
fbshipit-source-id: b4030bea5d92fa87f182a70d31f7e9d9e74989a9
Summary:
AsyncVfs provides async vfs interface.
It will be used in the native checkout instead of current use case that spawns blocking tokio tasks for VFS action
Reviewed By: quark-zju
Differential Revision: D26801250
fbshipit-source-id: bb26c4fc8acac82f4b55bb3f2f3964a6d0b64014
Summary:
Turned out that some of our commit cloud commits dont have an entry in changesets table,
but they do have an entry in bonsai_hg_mapping. It happens because of transient mysql errors.
Because of the mess we have in how we determine if a given hg changeset is present in mononoke or not,
this broken commit is considered backed up by commit cloud, while in reality its impossible to pull this commit.
This diff fixes it by first inserting changeset entry, and then bonsai_hg_mapping.
This way if an insertion of a changeset entry fails then the commit is not
considered to exist at all.
If an insertion of bonsai hg mapping fails, then commit is considered to be inserted but the whole
operation (it can be "hg cloud sync" for example) is considered failed. In this case hg client will
try to reupload this commit later.
But there's one caveat.
Hg changesets and bonsai changesets are not always round-trippable i.e.
generating from bonsai to hg and hg to bonsai might not always
produce the same results. So it's possible to push a commit which inserts changeset entry but not hg bonsai entry,
then we accidentally derive hg changeset from this bonsai and it's different from what user has pushed to mononoke.
This could lead to problems later (i.e. re-pushing the same commit again later would fail).
Another solution (inserting bonsai hg mapping and changeset entry in the same transaction) could make this issue
less likely, but it won't eliminate it completely. We discussed it, and I think it probably make sense to go
with the simpler fix first, since this problem should happen rarely in practice
(and if it happens often then we should prioritize fixing it).
Reviewed By: krallin
Differential Revision: D26845779
fbshipit-source-id: 61f8b0d835dc4aa0130e6be632c43307ff4a771f
Summary:
For dependencies V2 puts "version" as the first attribute of dependency or just after "package" if present.
Workspace section is after patch section in V2 and since V2 autoformats patch section then the third-party/rust/Cargo.toml manual entries had to be formatted manually since V1 takes it as it is.
The thrift files are to have "generated by autocargo" and not only "generated" on their first line. This diff also removes some previously generated thrift files that have been incorrectly left when the corresponding Cargo.toml was removed.
Reviewed By: ikostia
Differential Revision: D26618363
fbshipit-source-id: c45d296074f5b0319bba975f3cb0240119729c92
Summary:
The changes (and fixes) needed were:
- Ignore rules that are not rust_library or thrift_library (previously only ignore rust_bindgen_library, so that binary and test dependencies were incorrectly added to Cargo.toml)
- Thrift package name to match escaping logic of `tools/build_defs/fbcode_macros/build_defs/lib/thrift/rust.bzl`
- Rearrange some attributes, like features, authors, edition etc.
- Authors to use " instead of '
- Features to be sorted
- Sort all dependencies as one instead of grouping third party and fbcode dependencies together
- Manually format certain entries from third-party/rust/Cargo.toml, since V2 formats third party dependency entries and V1 just takes them as is.
Reviewed By: zertosh
Differential Revision: D26544150
fbshipit-source-id: 19d98985bd6c3ac901ad40cff38ee1ced547e8eb
Summary:
For streams we have `FbStreamExt` and `FbTryStreamExt`. Let's be a little
consistent and do the same with the future extension trait.
Reviewed By: StanislavGlebik
Differential Revision: D26485589
fbshipit-source-id: 5ebbda11d02e16709958a99a806aa70a8354672e
Summary: There was unlimited `try_join_all` call during parents_check. With a big number of parents to check (i.e. `6fa5806aefccf8f6c5500d943ce214f1aaf170ce` in www) it may affect performance. In case of S3 blobstore it leads to huge increase of TCP connections. Let's limit it to 100 concurrent futures.
Reviewed By: krallin
Differential Revision: D26455123
fbshipit-source-id: 6d84831ea0de437527e1abd234a05ca4ba03c01d
Summary:
There are existing metrics within blobimport, but it is not exported. Also there is scuba logging during commit which we can use to monitor backup blobimport process for large repos, which cause some trouble.
In the next diff in the stack I'm updating tw spec to add thrift port.
Reviewed By: StanislavGlebik
Differential Revision: D26452381
fbshipit-source-id: f5f0056760673e323d4e327327479ca5c95ff15c
Summary:
Autocargo V2 will use a more structured format for autocargo field
with the help of `cargo_toml` crate it will be easy to deserialize and handle
it.
Also the "include" field is apparently obsolete as it is used for cargo-publish (see https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields). From what I know this might be often wrong, especially if someone tries to publish a package from fbcode, then the private facebook folders might be shipped. Lets just not set it and in the new system one will be able to set it explicitly via autocargo parameter on a rule.
Reviewed By: ahornby
Differential Revision: D26339606
fbshipit-source-id: 510a01a4dd80b3efe58a14553b752009d516d651
Summary:
krallin noticed that we aren't using warm bookmark cache anymore. Turned out
the reason was in the fact that client uses `listkeyspatterns` call to fetch
bookmarks and not `listkeys`. This diff makes `listkeyspatterns` use warm
bookmark cache as well.
Reviewed By: markbt
Differential Revision: D26124605
fbshipit-source-id: 637db8d66934cabc1793f9f615fefddd07c3af62
Summary:
Lots of generated code in this diff. Only code change was in
`common/rust/cargo_from_buck/lib/cargo_generator.py`.
Path/git-only dependencies (ie `mydep = { path = "../foo/bar" }`) are not
publishable to crates.io. However, we are allowed to specify both a path/git
_and_ a version. When building locally, the path/git is chosen. When publishing,
the version on crates.io is chosen.
See https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations .
Note that I understand that not all autocargo projects are published on crates.io (yet).
The point of this diff is to allow projects to slowly start getting uploaded.
The end goal is autocargo generated `Cargo.toml`s that can be `cargo publish`ed
without further modification.
Reviewed By: lukaspiatkowski
Differential Revision: D26028982
fbshipit-source-id: f7b4c9d4f4dd004727202bd98ab10e201a21e88c
Summary:
When we tried to update to Tokio 0.2.14, we hit lots of hangs. Those were due
to incompatibilities between Tokio 0.2.14 and Futures 1.29. We fixed some of
the bugs (and others had been fixed and were pending a release), and Futures
1.30 have now been released, which unblocks our update.
This diff updates Tokio accordingly (the previous diff in the stack fixes an
incompatibility).
The underlying motivation here is to ease the transition to Tokio 1.0.
Ultimately we'll be pulling in those changes one or way or another, so let's
get started on this incremental first step.
Reviewed By: farnz
Differential Revision: D25952428
fbshipit-source-id: b753195a1ffb404e0b0975eb7002d6d67ba100c2
Summary:
This feature is useful for testing time-dependent stuff (e.g. it
allows you to stop/forward time). It's already included in the buck build.
Reviewed By: SkyterX
Differential Revision: D25946732
fbshipit-source-id: 5e7b69967a45e6deaddaac34ba78b42d2f2ad90e
Summary: Convert `Changsets` trait and all its uses to new type futures
Reviewed By: krallin
Differential Revision: D25638875
fbshipit-source-id: 947423e2ee47a463861678b146641bcc6b899a4a
Summary:
Like it says in the title. This is nice to do because we had old futures
wrapping new futures here, so this lets us get rid of a lot of cruft.
Reviewed By: ahornby
Differential Revision: D25502648
fbshipit-source-id: a34973b32880d859b25dcb6dc455c42eec4c2f94
Summary: Convert all BlobRepoHg methods to new type futures
Reviewed By: StanislavGlebik
Differential Revision: D25471540
fbshipit-source-id: c8e99509d39d0e081d082097cbd9dbfca431637e
Summary:
Right now, when we upload a hg commit, we check that we have all the content
the client is referencing.
The only problem is we do this by checking that the filenodes the client
mentions exist, but the way we store filenodes right now is we write them
concurrently with content blobs, so it is in fact possible to have a filenode
that references a piece of content that doesn't actually exist.
That isn't quite what one might call satisfactory when it comes to checking the
content does in fact exist, so this diff updates our content checking
In practice, with the way Mononoke works right now this should be quite free:
the client uploads everything all the time, and then we check later, so this
will just hit in the blobstore cache.
In a future world where clients don't upload stuff they already know we have,
that could be slower, but doing it the way is we do it before is simply not
correct, so that's not much better. The ways to make it faster would be:
- Trust that we'll hit in cache when checking for presence (could work out).
- Have the client prove to us that we have this content, and thread it through.
To do the latter, IMO the code here could actually look at the entries that
were actually uploaded, and not check them for presence again, but right now we
have a few layers of indirection that make this a bit tricky (technically, when
`process_one_entry` gets called, that means "I uploaded this", but nothing in
the signature of any of the functions involved really enforces that).
Reviewed By: StanislavGlebik
Differential Revision: D25422596
fbshipit-source-id: 3cf34d38bd6ed1cd83d93c778f04395c942b26c0
Summary: Also added a TryShared future to futures_ext. The problem with regular Shared is that if you want to share anyhow::Result the Error part of it is not cloneable. This TryShared will work nicely when returning anyhow::Result, which most of our code does.
Reviewed By: aslpavel
Differential Revision: D25223317
fbshipit-source-id: cf21141701884317a87dc726478dcd7a5a820c73
Summary: convert blobrepo:blobrepo_common to new type futures
Reviewed By: StanislavGlebik
Differential Revision: D25245426
fbshipit-source-id: d3db0e417545b2b0043e48a536737586005ac4bb
Summary: convert mercurial_derived_data to new type futures
Reviewed By: ahornby
Differential Revision: D25220329
fbshipit-source-id: c2532a12e915b315fe6eb72f122dbc37822bbb2a
Summary:
This diff prepares the Mononoke codebase for composition-based extendability of
`ScubaSampleBuilder`. Specifically, in the near future I will add:
- new methods for verbose scuba logging
- new data field (`ObservabilityContext`) to check if verbose logging should
be enabled or disabled
The higher-level goal here is to be able to enable/disable verbose Scuba
logging (either overall or for certain slices of logs, like for a certain
session id) in real time, without restarting Mononoke. To do so, I plan to
expose the aforementioned verbose logging methods, which will run a check
against the stored `ObservabilityContext` and make a decision of whether the
logging is enabled or not. `ObservabilityContext` will of course hide
implementation details from the renamed `ScubaSampleBuilderExt`, and just provide a yes/no
answer based on the current config and sample fields.
At the moment this should be a completely harmless change.
Reviewed By: krallin
Differential Revision: D25211089
fbshipit-source-id: ea03dda82fadb7fc91a2433e12e220582ede5fb8
Summary:
HgBlobEntry is kind of a problematic type right now:
- It has no typing to indicate if it's a file or a manifest
- It always has a file type, but we only sometimes care about it
This results in us using `HgBlobEntry` to sometimes represent
`Entry<HgManifestId, (FileType, HgFileNodeId)>`, and some other times to
represent `Entry<HgManifestId, HgFileNodeId>`.
This makes code a) confusing, b) harder to refactor because you might be having
to change unrelated code that cares for the use case you don't care about (i.e.
with or without the FileType), and c) results in us sometimes just throwing in
a `FileType::Normal` because we don't care about the type, which is prone to
breaking stuff.
So, this diff just removes it, and replaces it with the appropriate types.
Reviewed By: farnz
Differential Revision: D25122291
fbshipit-source-id: e9c060c509357321a8059d95daf22399177140f1
Summary: Remove 'static requirement for async methods of Blobstore, propagate this change and fixup low hanging fruits where the code can become 'static free easily.
Reviewed By: ahornby, farnz
Differential Revision: D24839054
fbshipit-source-id: 5d5daa04c23c4c9ae902b669b0a71fe41ee6dee6
Summary: It is possible that hash of newly created bonsai_changeset will be different from what is in prod repo. In this case let's fetch bonsai from the prod, to make backup repo consistent with prod.
Reviewed By: StanislavGlebik
Differential Revision: D24593003
fbshipit-source-id: 70496c59927dae190a8508d67f0e3d5bf8d32e5c
Summary: As part of the effort to deprecate futures 0.1 in favor of 0.3 I want to create a new futures_ext crate that will contain some of the extensions that are applicable from the futures_01_ext. But first I need to reclame this crate name by renaming the old futures_ext crate. This will also make it easier to track which parts of codebase still use the old futures.
Reviewed By: farnz
Differential Revision: D24725776
fbshipit-source-id: 3574d2a0790f8212f6fad4106655cd41836ff74d
Summary:
- convert ManifestOps to new style futures
- at this point `//eden/manifest:manifest` crate is completely free from old style futures
Reviewed By: krallin
Differential Revision: D24502214
fbshipit-source-id: f1cdb11bd8234f22af5c905243f71e1e9fca11f1
Summary:
Our case conflict checking is very inefficient on large changesets. The root
cause is that we traverse the parent manifest for every single file we are
modifying in the new changeset.
This results in very poor performance on large changes since we end up
reparsing manifests and doing case comparisons a lot more than we should. In
some pathological cases, it results in us taking several *minutes* to do a case
conflict check, with all of that time being spent on CPU lower-casing strings
and deserializing manifests.
This is actually a step we do after having uploaded all the data for a commit,
so this is pure overhead that is being added to the push process (but note it's
not part of the pushrebase critical section).
I ended up looking at this issue because it is contributing to the high
latencies we are seeing in commit cloud right now. Some of the bundles I
checked had 300+ seconds of on-CPU time being spent to check for case
conflicts. The hope is that with this change, we'll get fewer pathological
cases, and might be able to root cause remaining instances of latency (or have
that finally fixed).
This is pretty easy to repro.
I added a binary that runs case conflict checks on an arbitrary commit, and
tested it on `38c845c90d59ba65e7954be001c1eda1eb76a87d` (a commit that I noted
was slow to ingest in commit cloud, despite all its data being present already,
meaning it was basically a no-op). The old code takes ~3 minutes. The new one
takes a second.
I also backtested this by rigging up the hook tailer to do case conflict checks
instead (P145550763). It is about the same speed for most commits (perhaps
marginally slower on some, but we're talking microseconds here), but for some
pathological commits, it is indeed much faster.
This notably revealed one interesting case:
473b6e21e910fcdf7338df66ee0cbeb4b8d311989385745151fa7ac38d1b46ef (~8K files)
took 118329us in the new code (~0.1s), and 86676677us in the old (~87 seconds).
There are also commits with more files in recent history, but they're
deletions, so they are just as fast in both (< 0.1 s).
Reviewed By: StanislavGlebik
Differential Revision: D24305563
fbshipit-source-id: eb548b54be14a846554fdf4c3194da8b8a466afe
Summary:
I'm reworking some of our case conflict handling, and as part of this, I'm
going to be using check_case_conflicts for all our checking of case conflicts,
and notably for the case where we introduce a new commit and check it against
its parent (which, right now, does not check for case conflicts).
To do this and provide a good user experience (i.e. indicate which files
conflicted and with what), I need `check_case_conflicts` to report what files
the change conflicts with. This is what this diff does.
This does mean a few more allocations, so I "paid those off" by updating our
case lowering to allocate one fewer Vec and one fewer String per MPathElement
being lowercased.
Reviewed By: StanislavGlebik
Differential Revision: D24305562
fbshipit-source-id: 8ac14466ba3e84a3ee3d9216a84c2d9125a51b86
