Summary:
Handling of mutable renames was incorrect for two reasons:
1) We didn't add an entry to history graph, so only a single changeset before
rename was returned. That was easy to "fix" (just add a new entry to history
graph), but...
2) ...all history operations now have to use a different path (the source of
the rename path).
To fix it let's track not just the changeset id, but also the path for the
given changeset id. Since the path can potentially be large I wrapped it into
Arc to avoid expensive clones.
Differential Revision: D30576342
fbshipit-source-id: a99f6269c34b0a0c626104ec47c9392f984328fb
Summary:
Currently we have a problem in tunables - if a tunables was added, and then
removed, then value of tunable is not renewed until the binary is restarted.
This is not great - it makes reverts harder (e.g. if a diff added "tunable =>
true", then reverting the diff won't make any difference until the server is
restarted. So in order to revert the value of the tunable we'd need to write a
diff that does "tunable => false" or restart the binaries, neither of which is
great).
This is counter-intutivite, and this diff fixes it. From what I can tell, this
problem affected only strings/bools/ints - per repo variable weren't affected.
Reviewed By: Croohand
Differential Revision: D30544003
fbshipit-source-id: 353970a259a7faa0682866aae9a5699b8a783b22
Summary:
To be precise, it's ordering by generation number and parent order i.e. we'd
like to show first parents ahead of second parents.
Note that parent ordering is very basic at the moment, and won't always order
commits correctly when both parents have the same generation numbers. We can
improve it in the future, but I believe it shouldn't be a big issue now.
Reviewed By: mitrandir77
Differential Revision: D30483089
fbshipit-source-id: 67e13b5757831d652b57d6ad42b6135005a0b621
Summary:
The diff is giant, but it's just a one-line change to add the
nested-values feature to slog, we just have a whole bunch of projects dependent
on slog.
Reviewed By: dtolnay
Differential Revision: D30351289
fbshipit-source-id: b6c1c896b06cbdf23b1f92c0aac9a97aa116085d
Summary:
Let's make it possible to query mutable renames from fastlog. For now this is a
very basic support i.e. we don't support middle-of-history renames, blame is
not supported etc.
Mutable rename logic is gated by a tunable, so we can roll it back quickly in
case of problems.
Reviewed By: ahornby
Differential Revision: D30279932
fbshipit-source-id: 0e8e329e8ab4d4980ab401bd103e6c97419d0f67
Summary:
Autocargo only allows 1 rust-library per Cargo.toml, but right now we have 3
per Thrift library so that doesn't work:
https://www.internalfb.com/intern/sandcastle/log/?instance_id=27021598231105145&step_id=27021602582167211&step_index=13&name=Run%20config
There's little benefit in Autocargo-ifying those rules anyway since they're of
use to Thrift servers and this doesn't work at all in our OSS builds, so let's
just see if we can just noop them. That'll make the crate not exist at all as a
dep, but even considering that it exists only to link to a C++ library that
Autocargo doesn'tk now how to build anyway, that seems OK?
drop-conflicts
Reviewed By: markbt
Differential Revision: D30304720
fbshipit-source-id: 047524985b2dadab8610267c05e3a1b3770e84e6
Summary: Previous diffs switched all our usage from chashmap to dashmap as dashmap upstream is more responsive. Now remove chashmap from the cargo vendoring.
Reviewed By: dtolnay
Differential Revision: D30046522
fbshipit-source-id: 111ef9375bd8095f8b7c95752ecbc1988fb0438d
Summary:
The patches to these crates have been upstreamed.
allow-large-files
Reviewed By: jsgf
Differential Revision: D29891894
fbshipit-source-id: a9f2ee0744752b689992b770fc66b6e66b3eda2b
Summary: This is needed to disable sync-queue lookups and second blobstores lookups later on.
Reviewed By: StanislavGlebik
Differential Revision: D29663435
fbshipit-source-id: abb5109de6063158a7ff0a116a5c1d336bfdb43f
Summary:
Currently `is_present` makes a blobstores lookup and in case it couldn't determine whether the key exists or not, it checks the sync-queue (in case the key was written recently) and then might check the multiplex stores again, then fails if still unsure. This brings unnecessary complications and makes the multiplex blobstore less reliable.
More details in: https://fb.quip.com/wOCeAhGx6Oa1
This diff allows us to get rid of the queue and second store lookups and move the decision-making to the callers. The new logic is under the tunable for the safer rollout.
*This diff is safe to land.*
Reviewed By: StanislavGlebik
Differential Revision: D29428268
fbshipit-source-id: 9fc286ed4290defe16d58b2b9983e3baaf1a3fe4
Summary:
This diff joins all previous diffs, by making redaction config be read from configerator by default, when created from RepoFactory.
The logic for fetching from XDB still exists, and can be used if a tunable is enabled, though this needs a service restart. I plan to leave it there as a possible fallback until we successfully add some real-world thing to the new redaction config and it works.
Reviewed By: StanislavGlebik
Differential Revision: D29033596
fbshipit-source-id: 4c5e97f542457dc25cf234d31182c3106173585d
Summary:
This is just updating the os_info crate to my fork with a fix for Centos
Stream: https://github.com/stanislav-tkach/os_info/pull/267
Reviewed By: quark-zju
Differential Revision: D29410043
fbshipit-source-id: 3642e704f5a056e75fee4421dc59020fde13ed5e
Summary:
Pull in a patch which fixes writing out an incorrect entsize for the
`SHT_GNU_versym` section:
ddbae72082
Reviewed By: igorsugak
Differential Revision: D29248208
fbshipit-source-id: 90bbaa179df79e817e3eaa846ecfef5c1236073a
Summary:
In the previous diffs we allowed disabling all derived data for a given repo.
Now let's also allow disabling it only for a single type.
Reviewed By: farnz
Differential Revision: D29031081
fbshipit-source-id: b5bddcde9b1710addfeb40c1acfb7ba2e0241e76
Summary:
We've had a few sevs where expensive derivation for a single repo overloaded
our backends. This diff adds a tunable that makes it possible to quickly cancel
all derivations for a given repo. And one important caveat - it doesn't only
cancel derivations for a given repo, it also cancels the initial commit
traversal stage which can be quite expensive.
Note that it cancells all derivations for a given repo, at the moment there's
no way to cancel deriving just a single derived data type. I'd like to add it,
but in the next diffs since it might require changing the tunables - they don't
support tuning a list of strings for a given repo. Besides, having a way to
quickly disable all derivations for a given repo might be useful anyway.
Reviewed By: farnz
Differential Revision: D29028444
fbshipit-source-id: 559917e0e25eb060fe5be77a412bfec4ac3f1779
Summary: It will be used in the next diff to disable only certain derived data types
Reviewed By: yancouto
Differential Revision: D29031082
fbshipit-source-id: 9621a5eec522e369fef2b78e94599a056eda5317
Summary: revert the zstd crates back to previous version
Reviewed By: johansglock
Differential Revision: D29038514
fbshipit-source-id: 3cbc31203052034bca428441d5514557311b86ae
Summary: Update to latest version. This includes a patch to async-compression crate from [my PR updating it](https://github.com/Nemo157/async-compression/pull/125), I will remove once the crate is released.
Reviewed By: mitrandir77
Differential Revision: D28897019
fbshipit-source-id: 07c72f2880e7f8b85097837d084178c6625e77be
Summary:
Previously even if two commits are unrelated (e.g. they are in two branches
that are not related to each other) we'd still derive them sequentially. This
diff makes it possible to derive them in parallel to speed up derivation.
A few notes:
1) There's a killswitch to disable parallel derivation
2) By default we derive at most 10 commits at the same time, but it can be
configured.
Reviewed By: farnz
Differential Revision: D28830311
fbshipit-source-id: b5499ad5ac179f73dc94ca09927ec9c906592460
Summary:
## Idea
When `fetch_skiplist_index` is called, it will also create a task in the background that every 2 minutes, reloads the skiplist from the blobstore using the same key.
So that we can change the skiplist, we use an `ArcSwap` around it. I noticed a similar thing is already done in [here](https://fburl.com/code/non7ht60), maybe that's something to refactor into a single abstraction later.
It's possible to configure interval and enable/disable via [`MononokeTunables`](https://www.internalfb.com/code/fbsource/[8db13bf20639]/fbcode/eden/mononoke/tunables/src/lib.rs?lines=67), defaulting to enabled with 120s interval (no particular reason for the chosen interval, I'm happy to change it).
This some stuff that I won't do by default because I'm not sure it's necessary but feel free to ask and I'll add it:
- Add random delay when waiting for next iteration, so all repos don't reload mostly in sync
Reviewed By: markbt
Differential Revision: D28685596
fbshipit-source-id: 6ce2388ba92da07d5a968c1bdc446b73211155d3
Summary:
I'd like to de-risk this rollout a bit. Right now, we depend on everything
properly inserting a `log_id` in the bookmarks table.
Reviewed By: StanislavGlebik
Differential Revision: D28569840
fbshipit-source-id: 9cf32303e7f5de2119925d391b1e19509f705d52
Summary:
We're expecting this to be refreshed every few seconds. If that does not
happen, there's a point at which querying the log is just not efficient
anymore, and if we get there we should just rebuild from scratch.
This does that.
Reviewed By: farnz
Differential Revision: D28505644
fbshipit-source-id: 18e6cec8d8534de742781e658005a09301d3aa5f
Summary:
Like it says in the title. This adds support for using a BookmarksSubscription
to update bookmarks. This is controlled by a killswitch.
One little downside here is that if the subscription falls too far behind, we
might have too much data to fetch from MySQL, so turning this on via a
killswitch isn't optimal.
In the next diff in this stack, I'll update the BookmarksSubscription to
support a max age on last refresh and have it rebuild from scratch when we hit
this. I still want to have a killswitch here for now though, because
re-creating a subscription is more expensive than our current bookmarks
fetching (it's 2 queries in a transaction instead of 1). The long-term
intention is to remove this, though.
Reviewed By: StanislavGlebik
Differential Revision: D28505642
fbshipit-source-id: eefd23bea095c9d0351795ab599beb51bfac373f
Summary:
The goal of this diff is to make gettreepack requests that need to fetch a lot
of entries from blobstore faster.
Some of the gettreepack requests take really long time if they need to fetch,
say, 60K blobs. While this number of blobs is relatively high, it certainly
shouldn't take 1.5 mins to finish.
Upon closer inspection it looks like the reason for this slowness is a
combination of tokio coop mechanism and extremely large buffer size. Disabling
tokio coop and/or decreasing the buffer size makes requests significantly
faster (though I have to admin I didn't dig deeper to understand why they cause
this slowness).
To mitigate the issue I decided to decrease the buffer size first. This is a small
and predictable change that will be very easy to revert with tunable. Besides
having a buffer size of >1K doesn't make a lot of sense anyway, because we have
1K cachelib shards. Also this 10K buffer size was set a long time ago
(D8610582 (3dcf0048d8)), and there weren't a lot of measurements done to justify this size.
From the summary it seems that the problem was in slow manifold fetches from
remote regions, but as I said above, setting it highger than 1K is not helpful
anyway.
Reviewed By: krallin
Differential Revision: D28536686
fbshipit-source-id: 5f0ea04af8ce0c54c79d9668d95f2a1ece10f047
Summary: Upstream crate has landed my PR for zstd 1.4.9 support and made a release, so can remove this patch now.
Reviewed By: ikostia
Differential Revision: D28221163
fbshipit-source-id: b95a6bee4f0c8d11f495dc17b2737c9ac9142b36
Summary: We needed only for the initial rollout, we can delete it now
Reviewed By: krallin
Differential Revision: D28226715
fbshipit-source-id: 20349e9775b71292604a4054162f026087511db8
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:
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:
Knowing the prepushrebase changeset id is required for retroactive_review.
retroactive_review checks landed commits, but verify_integrity hook runs on a commit before landing. This way the landed commit has no straightforward connection with the original one and retroactive_review can't acknowledge if verify_integrity have seen it.
Reviewed By: krallin
Differential Revision: D27911317
fbshipit-source-id: f7bb0cfbd54fa6ad2ed27fb9d4d67b9f087879f1
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:
This updates the bookmarks cache TTL to be something we configure using
tunables instead of repo configs. There's a few goals here:
- Less us tune the pressure we put on SQL via a tunable
- Letting us experiment more easily with disabling this cache and tuning the
WBC poll interval.
Right now, this defaults to what we had set up in all our repo configs, which
is 2000ms.
Reviewed By: farnz
Differential Revision: D27915403
fbshipit-source-id: 4361d38939e5b2a0fe37442ed8c1dd0611af5098
Summary:
Like it says in the title. It would be a good idea for us to have support for
tweaking this so that we can control the load it forces onto our backend
storage.
Reviewed By: StanislavGlebik
Differential Revision: D27909638
fbshipit-source-id: 1783e1fddbbd609877ff3a9b3084f12e005fca4b
Summary: I am removing this change because we've decided to store prepushrebase changeset id server-side.
Reviewed By: ikostia
Differential Revision: D27853518
fbshipit-source-id: 888897bc48c67477309b09af5f8c1825ce20cbca
Summary:
After doing some local benchmarking (using MononokeApi instantiation as the
benchmark), one thing that's apparent is that we have quite a few parameters
here and that tuning them is likely to be a challenge.
One parameter in particular is the batch "objective", which controls how many
requests we want to see in the last batching interval before we choose to
batch (this is `rendezvous_dispatch_min_threshold`).
The problem with this is this is that there is no good, real-world, metric to
set it based on. This in contrast to the other parameters we have, which do
have some reasonable metric to compare to:
- rendezvous_dispatch_delay_ms: this is overhead we add to queries, so it
should be small & on the order of query execution latency (i.e. a few ms).
- rendezvous_dispatch_max_threshold: this controls how big our batches get, so
it should be on the order of what makes a SQL query too big (i.e. less than
a hundred records).
In contrast, we want to set `rendezvous_dispatch_min_threshold` such that
batching kicks in before we start using too many concurrent connections (which
is what query batching seeks to reduce), but the problem is that those two
numbers aren't directly connected. One clear problem, for example, is that if
our DB is in-region vs. out of-region, then for a given query execution time,
and a desired concurrency level before batching kicks in, we'd need different
values of `rendezvous_dispatch_min_threshold` (it would have to kick in faster
for the out-of-region workload).
So, this diff updates rendez vou to actually track concurrent connection count
before we force batching. This is the actual metric we care about here, and it
has a pretty natural "real world" values we can look at to decide where to set
it (our connection pool — which is limited at 100 concurrent connections —, and
our open connection baseline).
Note: I set this at 5 because that's more or less what servers look like
outside of spikes for Bonsai hg mapping, and of Changesets where I'm planning to
introduce this in the future:
- bonsai: https://fburl.com/ods/6d4a9qb5
- changesets: https://fburl.com/ods/kuq5x1vw (note: to make sense of this,
focus on just one server, otherwise the constnat spikes we get sort of hide
the big picture).
Reviewed By: farnz
Differential Revision: D27792603
fbshipit-source-id: 1a9189f6b50d48444b3373bd1cb14dc51b85a6d2
Summary:
Like it says in the title, this adds support for publishing our max open
connections to ODS. Note that this is a little more involved than I would like
for it to be, but there is no way to get direct access to this information.
This means, we need to:
- Expose how many open connections we have in flight (this is done earlier in
this stack in the Rust MySQL bindings).
- Periodically get this information out out for MySQL, put it in a timeseries.
- Get the max out of said timeseries and publish it to a counter so that it can
be fetched in ODS.
This is what this diff does. Note that I've only done this for read pools,
largely because I think they're the ones we tend to exhaust the most and I'd
like to see if there is value in exposing those counters before I use them.
We do the aggregation on a dedicated thread here. I contemplated making this a
Tokio task, but I figured making it a thread would make it easier to see if
it's misbehaving in any way (also: note that the SQL client allocates a bunch
of threads already anyway).
Reviewed By: HarveyHunt
Differential Revision: D27678955
fbshipit-source-id: c7b386f3a182bae787d77e997d108d8a74a6402b
