Summary:
The dbbookmarks crate is getting too large for a single file. Split it up into
a `store` module, which implements the bookmarks traits, and a `transaction`
module, which handles bookmark transactions.
Reviewed By: krallin
Differential Revision: D22437088
fbshipit-source-id: 629b62de151400cdbf56d502aef061df46c3da81
Summary:
Separate out the `BundleReplayData` from the `BookmarkUpdateReason` enum. There's
no real need for this to be part of the reason, and removing it means we can
abstract away the remaining dependency on Mercurial changeset IDs from
the main bookmarks traits.
Reviewed By: mitrandir77, ikostia
Differential Revision: D22417659
fbshipit-source-id: c8e5af7ba57d10a90c86437b59c0d48e587e730e
Summary: For populating the XDB blobstore, we'd like to copy data from Manifold - the easiest way to do that is to exploit MultiplexedBlobstore's scrub mode to copy data directly.
Reviewed By: krallin
Differential Revision: D22373838
fbshipit-source-id: 550a9c73e79059380337fa35ac94fe1134378196
Summary:
Previously backfill_batch_dangerous method was calling internal derive_impl() method
directly. That wasn't great (after all, we are calling a function whose name suggests it should only be called from inside derive data crate) and this diff changes it so that we call batch_derive() method instead.
This gives a few benefits:
1) We no longer call internal derive_impl function
2) It allows different types of derived data to override batching behaviour.
For example, we've already overriden it for fsnodes and next diff will override
it for blame as well.
To make it compatible with derive_impl() batch_derive() now accepts derive data mode and mapping
Reviewed By: krallin
Differential Revision: D22435044
fbshipit-source-id: a4d911606284676566583a94199195860ffe2ecf
Summary:
D22206317 (9a6ed4b6ca) added requesting of predecessor information for suspected primordials
by the successor ID. This allows recovery of earlier predecessors when partial
data upload resulted in the history of a commit being extended backwards.
Unfortunately, while the individual requests are fast, the combined request
using `OR` in SQL ended up being very slow for some requests.
Separate out the requests at the application level, and aggregate the results
by concatenating them. `collect_entries` already handles duplicates should any
arise.
Most of the time the successor query will very quickly return no rows, as
it only matters when history is extended backwards, which is expected to be
rare.
Reviewed By: ikostia
Differential Revision: D22456062
fbshipit-source-id: 1e6094b4ac1590a5824e9ae6ef48468766560188
Summary:
Bypass truncation-based transaction if narrow-heads is on.
The transaction abort still works logically because commit references stay
unchanged on abort.
Related EdenFS and Mononoke tests are updated. Mononoke tests probably
shouldn't rely on revlog / fncache implementation details in hg.
Reviewed By: DurhamG
Differential Revision: D22240186
fbshipit-source-id: f97efd60855467b52c9fb83e7c794ded269e9617
Summary:
The goal is to make it easier to implement unit tests, which depend on `LiveCommitSyncConfig`. Specifically, `scs` has a piece of code, which instantiates `mononoke_api::Repo` with a test version of `CommitSyncConfig`. To migrate it to `LiveCommitSyncConfig`, I need to be able to create a test version of that. It **is** possible now, but would require me to turn a supplied instance of `CommitSyncConfig` back into `json`, which is cumbersome. Using a `dyn LiveCommitSyncConfig` there, instead of a concrete struct seems like a good idea.
Note also that we are using this technique in many places: most (all?) of our DB tables are traits, which we then implement for SQL-specific structs.
Finally, this diff does not actually migrate all of the current users of `LiveCommitSyncConfig` (the struct) to be users of `LiveCommitSyncConfig` (the trait), and instead makes them use `CfgrLiveCommitSyncConfig` (the trait impl). The idea is that we can migrate bits to use traits when needed (for example, in an upcoming `scs` diff). When not needed, it's fine to use concrete structs. Again, this is already the case in a a few places: we sometimes use `SqlSyncedCommitMapping` struct directly, instead of `T: SyncedCommitMapping` or `dyn SyncedCommitMapping`.
Reviewed By: StanislavGlebik
Differential Revision: D22383859
fbshipit-source-id: 8657fa39b11101684c1baae9f26becad6f890302
Summary:
This updates the AsyncRead implementations we use in hgproto and
mercurial_bundles to use a LimitedAsyncRead. The upshot of this change is that
we eliminate O(N^2) behavior when parsing the data we receive from clients.
See the earlier diff on this stack for more detail on where this happens, but
the bottom line is that Framed presents a full-size buffer that we zero out
every time we try to read data. With this change, the buffer we zero out is
comparable to the amount of data we are reading.
This matters in commit cloud because bundles might be really big, and a single
big bundle is enough to take an entire core for a spin or 20 minutes (and they
achieve nothing but time out in the end). That being said, it's also useful for
non-commit cloud bundles: we do occasionally receive big bundles (especially
for WWW codemods), and those will benefit from the exact same speedup.
One final thing I should mention: this is all in a busy CPU poll loop, and as I noted
in my earlier diff, the effect persists across our bundle receiving code. This means
it will sometimes result in not polling other futures we might have going.
Reviewed By: farnz
Differential Revision: D22432350
fbshipit-source-id: 33f1a035afb8cdae94c2ecb8e03204c394c67a55
Summary:
I want to update the health check to stop averaging averages (like in
D22394014). To do this, I need those counters.
Reviewed By: ahornby
Differential Revision: D22410196
fbshipit-source-id: aa5cbfe6607be3b953887f1639e1de54baac7389
Summary:
Just knowing the number of fetched undesired files doesn't give the full
picture. e.g. fetching lots of small files is better than fetching single
multi-Gb file.
So knowing the size of files is helpful
Reviewed By: krallin
Differential Revision: D22408400
fbshipit-source-id: 7653c1cdceccf50aeda9ce8a4880ee5178d4b107
Summary: D22381744 updated the version of `futures` in third-party/rust to 0.3.5, but did not regenerate the autocargo-managed Cargo.toml files in the repo. Although this is a semver-compatible change (and therefore should not break anything), it means that affected projects would see changes to all of their Cargo.toml files the next time they ran `cargo autocargo`.
Reviewed By: dtolnay
Differential Revision: D22403809
fbshipit-source-id: eb1fdbaf69c99549309da0f67c9bebcb69c1131b
Summary:
Sometimes you want to fetch a file. Using curl and the LFS server works, but
this really should be part of Mononoke admin.
Reviewed By: ikostia
Differential Revision: D22397472
fbshipit-source-id: 17decf4aa2017a2c1be52605a254692f293d1bcd
Summary:
This got broken when we moved to Tokio 0.2. Let's fix it and add a test to make
sure it does not regress.
Reviewed By: ikostia
Differential Revision: D22396261
fbshipit-source-id: a8359aee33b4d6d840581f57f91af6c03125fd6a
Summary:
This diff adds two new bits of functionality to `LiveCommitSyncConfig`:
- getting all possible versions of `CommitSyncConfig` for a given repo
- getting `CommitSyncConfig` for a repo by version name
These bits are meant to be used in:
- `commit_validator` and `bookmarks_validator`, which would
need to run validation against a specific config version
- `mononoke_admin`, which would need to be able to query all versions,
display the version used to sync two commits and so on
Reviewed By: StanislavGlebik
Differential Revision: D22235381
fbshipit-source-id: 42326fe853b588849bce0185b456a5365f3d8dff
Summary:
This updates the virtually_sharded_blobstore to deduplicate puts only if the
data being put is actually the data we have put in the past. This is done by
keeping track of the hash of things we've put in the presence cache.
This has 2 benefits:
- This is safer. We only dedupe puts we 100% know succeeded (because this
particular instance was the one to attempt the put).
- This is creates less surprises, notably it lets us overwrite data in the
backing store (if we are writing something different).
Reviewed By: StanislavGlebik
Differential Revision: D22392809
fbshipit-source-id: d76a49baa9a5749b0fb4865ee1fc1aa5016791bc
Summary:
Running those on my devserver, I noticed they can be a bit flaky. They're are
racy on the purpose, but let's relax them a bit.
We have a lot of margin here — our blobstore is rate limited at once request
every 10ms, and we need to do 100 requests (the goal is to show that they don't
all wait), so 100ms is fine to prove that they're not rate limited when sharing
the same data.
Reviewed By: StanislavGlebik
Differential Revision: D22392810
fbshipit-source-id: 2e3c9cdf19b0e4ab979dfc000fbfa8da864c4fd6
Summary:
When we look up how a commit was synced, we frequently need to know which version of `CommitSyncConfig` was used to sync it. Specifically, this is useful for admin tooling and commit validator, which I am planning to migrate to use versioned `CommitSyncConfig` in the near future.
Later I will also include this information into `RewrittenAs` variant of `CommitSyncOutcome`, so that we expose it to real users. I did not do it in this diff to keep is small and easy to review. And because the other part is not ready :P
Reviewed By: StanislavGlebik
Differential Revision: D22255785
fbshipit-source-id: 4312e9b75e2c5f92ba018ff9ed9149efd3e7b7bc
Summary: When I've implemented this method I didn't test it for preserving the order of the input changesets and I've noticed my mistake when I was testing the scmquery part.
Reviewed By: StanislavGlebik
Differential Revision: D22374981
fbshipit-source-id: 4529f01370798377b27e4b6a706fc192a1ea928e
Summary:
Add the `scsc list-bookmarks` command, which lists bookmarks in a repository.
If a commit id is also provided, `list-bookmark` will be limited to bookmarks that
point to that commit of one of its descendants.
Reviewed By: mitrandir77
Differential Revision: D22361240
fbshipit-source-id: 17067ba47f9285b8137a567a70a87fadcaabec80
Summary:
There is inevitably interaction between caching, deduplication and rate
limiting:
- You don't want the rate limiting to be above caching (in the blobstore stack,
that is), because you shouldn't rate limits cache hits (this is where we are
today).
- You don't want the rate limiting to below deduplication, because then you get
priority inversion where a low-priority rate-limited request might hold the
semaphore while a higher-priority, non rate limited request wants to do the
same fetch (we could have moved rate limiting here prior to introducing
deduplication, but I didn't do it earlier because I wanted to eventually
introduce deduplication).
So, now that we have caching and deduplication in the same blobstore, let's
also incorporate rate limiting there!.
Note that this also brings a potential motivation for moving Memcache into this
blobstore, in case we don't want rate limiting to apply to requests before they
go to the _actual_ blobstore (I did not do this in this diff).
The design here when accessing the blobstore is as follows:
- Get the semaphore
- Check if the data is in cache, if so release the semaphore and return the
data.
- Otherwise, check if we are rater limited.
Then, if we are rate limited:
- Release the semaphore
- Wait for our turn
- Acquire the semaphore again
- Check the cache again (someone might have put the data we want while we were
waiting).
- If the data is there, then return our rate limit token.
- If the data isn't there, then proceed to query the blobstore.
If we aren't rate limited, then we just proceed to query the blobstore.
There are a couple subtle aspects of this:
- If we have a "late" cache hit (i.e. after we waited for rate limiting), then
we'll have waited but we won't need to query the blobstore.
- This is important when a large number of requests from the same key
arrive at the same time and get rate limited. If we don't do this second
cache check or if we don't return the token, then we'll consume a rate
limiting token for each request (instead of 1 for the first request).
- If a piece of data isn't cacheable, we should treat it like a cache hit with
regard to semaphores (i.e. release early), but like a miss with regard to
rate limits (i.e. wait).
Both of those are addressed captured in the code by returning the `Ticket` on a
cache hit. We can then choose to either return the ticket on a cache hit, or wait
for it on a cache miss.
(all of this logic is captured in unit tests, we can remove any of the blocks
there in `Shards::acquire` and a test will fail)
Reviewed By: farnz
Differential Revision: D22374606
fbshipit-source-id: c3a48805d3cdfed2a885bec8c47c173ee7ebfe2d
Summary:
Sometimes we take a token then realize we don't want it. In this case, giving it back is convenient.
This adds this!
Reviewed By: farnz
Differential Revision: D22374607
fbshipit-source-id: ccf47e6c75c37d154704645c9e826f514d6f49f6
Summary:
This is a mirror image of a diff, which made backsyncer use `LiveCommitSyncConfig`: we want to use configerator-based live configs, when we run in the continuous tailing mode.
As no-op iteration time used to be 10s and that's a bit wasteful for tests, this diff changes it to be configurable.
Finally, because of instantiating various additional `CommitSyncerArgs` structs, this diff globs out some of the `using repo` logs (which aren't very useful as test signals anyway, IMO).
Reviewed By: StanislavGlebik
Differential Revision: D22209205
fbshipit-source-id: fa46802418a431781593c41ee36f468dee9eefba
Summary: This diff actually start to use the option
Reviewed By: krallin
Differential Revision: D22373943
fbshipit-source-id: fe23da9c3daa1f9f91a5ee5e368b33e0091aa9c1
Summary:
Previously if a blame request was rejected (e.g. because a file was too large)
then we returned BlameError::Error.
This doesn't look correct, because there's BlameError::Rejected. This diff
makes it so that fetch_blame function returns BlameError::Rejected
Reviewed By: aslpavel
Differential Revision: D22373948
fbshipit-source-id: 4859809dc315b8fd66f94016c6bd5156cffd7cc2
Summary:
In the next diffs we'll need to read override_blame_filesize_limit from derived
data config, and this config is stored in BlobRepo.
this diff makes a small refactoring to pass BlobRepo to fetch_full_file_content
Reviewed By: krallin
Differential Revision: D22373946
fbshipit-source-id: b209abce82c0279d41173b5b25f6761659a92f3d
Summary: This will make adding blame file size limit override the next diffs easier
Reviewed By: krallin
Differential Revision: D22373945
fbshipit-source-id: 4857e43c5d80596340878753ea90bf31d7bb3367
Summary:
We're always yielding zero or one child during traversal, bounded traversal is
unnecessary here
Differential Revision: D22242148
fbshipit-source-id: b4c8a1279ef7bd15e9d0b3b2063683f45e30a97a
Summary:
Let's use new option in CLI. Unfortunately we can't easily accept commit ids in
named params so it has to be a postional one.
Differential Revision: D22234412
fbshipit-source-id: a9c27422fa65ae1c42cb1c243c7694507a957437
Summary:
If anything were to go wrong, we'd be happy to know which puts we ignored. So,
let's log them.
Reviewed By: farnz
Differential Revision: D22356714
fbshipit-source-id: 5687bf0fc426421c5f28b99a9004d87c97106695
Summary:
Eventually, I plan to make this the default, but for now I'd like to make it
something we can choose to turn on or off as a cmd argument (so we can start
with the experimental tier and Fastreplay).
Note that this mixes volatile vs. non-volatile pools when accessing the pools
for cacheblob. In practice, those pools are actually volatile, it's just that
things don't break if you access them as non-volatile.
Reviewed By: farnz
Differential Revision: D22356537
fbshipit-source-id: 53071b6b21ca5727d422e10f685061c709114ae7
Summary:
I canaried this on Fastreplay, but unfortunately that showed that sometimes we
just deadlock, or get so slow we might as well be deadlocked (and it happens
pretty quickly, after ~20 minutes). I tried spawning all the `get()` futures,
and that fixes the problem (but it makes gettreepack noticeably slower), so
that suggests something somewhere is creating futures, polling them a little
bit, then never driving them to completion.
For better or worse, I'd experienced the exact same problem with the
ContextConcurrencyBlobstore (my initial attempt at QOS, which also used a
semaphore), so I was kinda expecting this to happen.
In a sense, this nice because I we've suspected there were things like that in
the codebase for a while (e.g. with the occasional SQL timeout we see where it
looks like MySQL responds fast but we don't actually poll it until past the
timeout), and it gives us a somewhat convenient repro.
In another sense, it's annoying because it blocks this work :)
So, to work around the problem, for now, let's spawn futures to force the work
to complete when a semaphore is held. I originally had an unconditional spawn
here, but that is too expensive for the cache-hit code path and slows things
down (by about ~2x).
However, having it only if we'll query the blobstore isn't not as expensive,
and that seems to be fine (in fact it is a ~20% p99 perf improvement,
though the exact number depends on the number of shard we use for this, which I've had to tweak a bit).
https://pxl.cl/1c18H
I did find what I think is one potential instance of this problem in
`bounded_traversal_stream`, which is that we never try to poll `scheduled` to
completion. Instead, we just poll for the next ready future in our
FuturesUnordered, and if that turns out to be synchronous work then we'll just
re-enqueue more stuff (and sort of starve async work in this FuturesUnordered).
I tried updating bounded traversal to try a fairer implementation (which polls
everything), but that wasn't sufficient to make the problem go away, so I think
this is something we have to just accept for now (note that this actually has
some interesting perf impact in isolation: it's a free ~20% perf improvement on
p95+: https://pxl.cl/1c192
see 976b6b92293a0912147c09aa222b2957873ef0df if you're curious
Reviewed By: farnz
Differential Revision: D22332478
fbshipit-source-id: 885b84cda1abc15c51fbc5dd34473e49338e13f4
Summary: Like it says in the title. Those are useful!
Reviewed By: farnz
Differential Revision: D22332479
fbshipit-source-id: f9bddad75fcbed2593c675f9ba45965bd87f1575
Summary:
The goal of this blobstore is to dedupe reads by waiting for them to finish and
hit cache instead (and also to dedupe writes, but that's not relevant here).
However, this is not a desirable feature if a blob cannot be stored in cache,
because then we're serializing accesses for no good reason. So, when that
happens, we store "this cannot be stored in cache", and we release reads
immediately.
Reviewed By: farnz
Differential Revision: D22285269
fbshipit-source-id: be7f1c73dc36b6d58c5075172e5e3c5764eed894
Summary:
I'm going to store things that aren't quite the exact blobs in here, so on the
off chance that we somehow have two caching blobstores (the old one and this
one) that use the same pools, we should avoid collisions by using a prefix.
And, since I'm going to use a prefix, I'm adding a newtype wrapper to not use
the prefixed key as the blobstore key by accident.
Differential Revision: D22285271
fbshipit-source-id: e352ba107f205958fa33af829c8a46896c24027e
Summary:
This introduces a caching blobstore that deduplicates reads and writes. The
underlying motivation is to improve performance for processes that might find
themsleves inadvertently reading the same data concurrently from a bunch of
independent callsites (most of Mononoke), or writing the same bit of data over
and over again.
The latter is particularly useful for things like commit cloud backfilling in
WWW, where some logger commits include the same blob being written hundreds or
thousands of times, and cause us to overload the underlying Zippy shard in
Manifold. This is however a problem we've also encountered in the past in e.g.
the deleted files manifest and had to solve there. This blobstore is a little
different in the sense that it solves that problem for all writers.
This comes at the cost of writes being dropped if they're known to be
redundant, which prevents updates through this blobstore. This is desirable for
most of Mononoke, but not all (notably, for skiplist updates it's not great).
For now, I'm going to add this behind an opt-in flag, and later on I'm planning
to make it opt-out and turn it off there (I'm thinking to use the CoreContext
for this).
Reviewed By: farnz
Differential Revision: D22285270
fbshipit-source-id: 4e3502ab2da52a3a0e0e471cd9bc4c10b84a3cc5
Summary: This allowed me to compare two alternative approaches to queue draining, and generally seems like a useful thing to do.
Reviewed By: krallin
Differential Revision: D22364733
fbshipit-source-id: b6c76295c85b4dec6f0bfd7107c30bb4e4a28942
Summary: It's useful to derive all enabled derived data at once
Reviewed By: krallin
Differential Revision: D22336338
fbshipit-source-id: 54bc27ab2c23c175913fc02e6bf05d18a54c249c
Summary:
We've recently added an option to perform a stack move in megarepolib. A "stack
move" it's a stack of commits that move a files according to a mover. Now let's
expose it in the megarepotool
Reviewed By: ikostia
Differential Revision: D22312486
fbshipit-source-id: 878d4b2575ed2930bbbf0b9b35e51bb41393e622
Summary:
Implement `commit_list_descendant_bookmarks` by iterating over all bookmarks and
checking if the target commit is an ancestor of the bookmark's current target.
Reviewed By: mitrandir77
Differential Revision: D22357988
fbshipit-source-id: e1b1d7387742ba7133370f52c4d36c0b1a77f4e3
Summary:
Currently the `move_bookmark` API needs to get the old bookmark location in order
to move the bookmark. We'll fix that in general later, but for now we need to
make sure the value we use doesn't come from an out-of-date cache (e.g. the
warm_bookmarks_cache), as it may prevent the move from working.
Reviewed By: krallin
Differential Revision: D22358467
fbshipit-source-id: 4d46a6be717644b24663318326fdcd81249481c9
Summary:
Bookmark requests that are truncated because the requested limit is reached now return a `continue_after` value, containing the last bookmark that was processed.
Callers can make a subsequent request with the same parameters, but `after` set to the value received in `continue_after` to continue their request where it left off.
Reviewed By: krallin
Differential Revision: D22338301
fbshipit-source-id: 81e398bee444e0960e65dc3b4cdbbe877aff926d
Summary:
Add `commit_list_descendant_bookmarks` which will list all bookmarks that are
descendants of a particular commit.
We will also use this opportunity to complete the implementation of pagination
for regular bookmark listing, so add the appropriate fields to the
`repo_list_bookmarks` request and response structs.
Reviewed By: StanislavGlebik
Differential Revision: D22338300
fbshipit-source-id: defd019795c2a2ac9e5573d58de187c10848397f
Summary:
Add a new parameter, `pagination`, to the `list` method of the `Bookmarks` trait.
This restricts the returned bookmarks to those lexicographically after the
given bookmark name (exclusive). This can be use to implement pagination:
callers can provide the last bookmark in the previous page to fetch the
next page of bookmarks.
Reviewed By: krallin
Differential Revision: D22333943
fbshipit-source-id: 686df545020d936095e29ae5fee24258511f4083
Summary:
Rework the bookmarks traits:
* Split out log functions into a separate `BookmarkUpdateLog` trait. The cache doesn't care about these methods.
* Simplify `list` down to a single method with appropriate filtering parameters. We want to add more filtering types, and adding more methods for each possible combination will be messier.
* The `Bookmarks` and `BookmarkUpdateLog` traits become `attributes` on `BlobRepo`, rather than being a named member.
Reorganise the bookmarks crate to separate out the bookmarks log and transactions into their own modules.
Reviewed By: krallin
Differential Revision: D22307781
fbshipit-source-id: 4fe514df8b7ef92ed3def80b21a16e196d916c64
Summary:
The LIKE pattern used by bookmark prefixes needs to be escaped, otherwise
users looking for bookmarks containing `\`, `_` or `%` will get the
wrong results.
Reviewed By: krallin
Differential Revision: D22336716
fbshipit-source-id: 99b0ad6097f096358e66042752e4d153359935be
Summary: We were monitoring the wrong lag so far.
Reviewed By: farnz
Differential Revision: D22356455
fbshipit-source-id: abe41a4154c2a8d53befed4760e2e9544797c845
