Summary: We've got multiple manifold parameters now, two of which are Option<i64>, so lets create a struct to name them
Reviewed By: HarveyHunt
Differential Revision: D30305462
fbshipit-source-id: 44eee00d478e4485d074a14fcccec2f0f9572ecd
Summary: This diff adds a CLI option to be able to override the manifold request priority for a particular job or command line run.
Reviewed By: HarveyHunt
Differential Revision: D30277209
fbshipit-source-id: 58217c11234133dfc68e11b230d99066dd783600
Summary:
Like it says in the title. The API between Bytes 1.x has changed a little bit,
but the concepts are basically the same, so we just need to change the
callsites that were calling `bytes()` and have them ask for `chunk()` instead.
This diff attempts to be as small as it can (and it's already quite big). I
didn't attempt to update *everything*: I only updated whatever was needed to
keep `common/rust/tools/scripts/check_all.sh` passing.
However, there are a few changes that fall out of this. I'll outline them here:
## `BufExt`
One little caveat is the `copy_to_bytes` we had on `BufExt`. This was
introduced into Bytes 1.x (under that name), but we can't use it here directly.
The reason we can't is because the instance we have is a `Cursor<Bytes>`, which
receives an implementation of `copy_from_bytes` via:
```
impl<T: AsRef<[u8]>> Buf for std::io::Cursor<T>
```
This means that implementation isn't capable of using the optimized
`Bytes::copy_from_bytes` which doesn't do a copy at all. So, instead, we need
to use a dedicated method on `Cursor<Bytes>`: `copy_or_reuse_bytes`.
## Calls to `Buf::to_bytes()`
This method is gone in Bytes 1.x, and replaced by the idiom
`x.copy_to_bytes(x.remaining())`, so I updated callsites of `to_bytes()`
accordingly.
## `fbthrift_ext`
This set of crates provides transports for Thrift calls that rely on Tokio 0.2
for I/O. Unfortunately, Tokio 0.2 uses Bytes 0.5, so that doesn't work well.
For now, I included a copy here (there was only one required, when reading from
the socket). This can be removed if we update the whole `fbthrift_ext` stack to
Bytes 1.x. fanzeyi had been wanting to update this to Tokio 1.x, but was blocked on `thrift/lib/rust` using Bytes 0.5, and confirmed that the overhead of a copy here is fine (besides, this code can now be updated to Tokio 1.x to remove the copy).
## Crates using both Bytes 0.5 & Bytes 1.x
This was mostly the case in Mononoke. That's no coincidence: this is why I'm
working on this. There, I had to make changes that consist of removing Bytes
0.5 to Bytes 1.x copies.
## Misuse of `Buf::bytes()`
Some places use `bytes()` when they probably mean to use `copy_to_bytes()`. For
now, I updated those to use `chunk()`, which keeps the behavior the same but
keeps the code buggy. I filed T91156115 to track fixing those (in all
likelihood I will file tasks for the relevant teams).
Reviewed By: dtolnay
Differential Revision: D28537964
fbshipit-source-id: ca42a614036bc3cb08b21a572166c4add72520ad
Summary: The new and new_with_ttl constructors were causing duplication. Combine them.
Reviewed By: farnz
Differential Revision: D28471162
fbshipit-source-id: c7095a9a337d0ccbf5cd15ac3650cd5b361aaebf
Summary:
These are undermaintained, and need an update for oncall support. Start by moving to CXX, which makes maintenance easier.
In the process, I've fixed a couple of oddities in the API that were either due to the age of the code, or due to misunderstandings propagating through bindgen that CXX blocks, and fixed up the users of those APIs.
Reviewed By: dtolnay
Differential Revision: D28264737
fbshipit-source-id: d18c3fc5bfce280bd69ea2a5205242607ef23f28
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:
There is a very frustrating operation that happens often when working on the
Mononoke code base:
- You want to add a flag
- You want to consume it in the repo somewhere
Unfortunately, when we need to do this, we end up having to thread this from a
million places and parse it out in every single main() we have.
This is a mess, and it results in every single Mononoke binary starting with
heaps of useless boilerplate:
```
let matches = app.get_matches();
let (caching, logger, mut runtime) = matches.init_mononoke(fb)?;
let config_store = args::init_config_store(fb, &logger, &matches)?;
let mysql_options = args::parse_mysql_options(&matches);
let blobstore_options = args::parse_blobstore_options(&matches)?;
let readonly_storage = args::parse_readonly_storage(&matches);
```
So, this diff updates us to just use MononokeEnvironment directly in
RepoFactory, which means none of that has to happen: we can now add a flag,
parse it into MononokeEnvironment, and get going.
While we're at it, we can also remove blobstore options and all that jazz from
MononokeApiEnvironment since now it's there in the underlying RepoFactory.
Reviewed By: HarveyHunt
Differential Revision: D27767700
fbshipit-source-id: e1e359bf403b4d3d7b36e5f670aa1a7dd4f1d209
Summary:
Basically every single Mononoke binary starts with the same preamble:
- Init mononoke
- Init caching
- Init logging
- Init tunables
Some of them forget to do it, some don't, etc. This is a mess.
To make things messier, our initialization consists of a bunch of lazy statics
interacting with each other (init logging & init configerator are kinda
intertwined due to the fact that configerator wants a logger but dynamic
observability wants a logger), and methods you must only call once.
This diff attempts to clean this up by moving all this initialization into the
construction of MononokeMatches. I didn't change all the accessor methods
(though I did update those that would otherwise return things instantiated at
startup).
I'm planning to do a bit more on top of this, as my actual goal here is to make
it easier to thread arguments from MononokeMatches to RepoFactory, and to do so
I'd like to just pass my MononokeEnvironment as an input to RepoFactory.
Reviewed By: HarveyHunt
Differential Revision: D27767698
fbshipit-source-id: 00d66b07b8c69f072b92d3d3919393300dd7a392
Summary: SQLBlob doesn't benefit from sharing a pool with other MySQL users, but does benefit from more aggressive connection timeouts. Give it its own pool, which we can tweak later.
Reviewed By: krallin
Differential Revision: D27651133
fbshipit-source-id: 8f5216ec0506b217f9365babfe1ebac00f68a9a9
Summary: It's no longer in use, so remove it completely to simplify the code
Reviewed By: ahornby
Differential Revision: D27234699
fbshipit-source-id: ec26f4e283d48b05e19b951b8485ca6fa7751072
Summary: this is useful so we can measure the effect of cachelib marshalling overhead in CacheBlob separately without zstd compression in play
Reviewed By: krallin
Differential Revision: D27043229
fbshipit-source-id: cf7e35688bdd96c029ee7858f59e46583726f271
Summary: Can use the ones from BlobstoreOptions rather than doing our own
Reviewed By: krallin
Differential Revision: D27043230
fbshipit-source-id: d3db19adaf8819709d069296dec955b2159d5546
Summary: Benchmark had a partial duplicate of the sqlblob opening code from blobstore factory but without unsharded support, so use factory instead.
Reviewed By: krallin
Differential Revision: D27060010
fbshipit-source-id: d1c5704cdec17e3d0b1b54538caf7a3893c3610f
Summary: Was fixed at 2 reads, add an option to allow testing read performance more throughly.
Reviewed By: farnz
Differential Revision: D27043234
fbshipit-source-id: 4bb5f49007a4fa67c42e872e236417fa5ce5c9a0
Summary: Tidy up a bit and use the constants
Reviewed By: krallin
Differential Revision: D27043233
fbshipit-source-id: 3208e2f35c67b4b22bb5f8189cd8c5b399604833
Summary:
The earlier diffs in this stack have removed all our dependencies on the Tokio
0.1 runtime environment (so, basically, `tokio-executor` and `tokio-timer`), so
we don't need this anymore.
We do still have some deps on `tokio-io`, but this is just traits + helpers,
so this doesn't actually prevent us from removing the 0.1 runtime!
Note that we still have a few transitive dependencies on Tokio 0.1:
- async-unit uses tokio-compat
- hg depends on tokio-compat too, and we depend on it in tests
This isn't the end of the world though, we can live with that :)
Reviewed By: ahornby
Differential Revision: D26544410
fbshipit-source-id: 24789be2402c3f48220dcaad110e8246ef02ecd8
Summary: In preparation for adding C++ client alternatives, use a wrapper around the Thrift client. This currently just calls the Thrift client directly, but will grow the ability to call the C++ client, too. For now, there's a dummy parameter to control the use of the C++ client.
Reviewed By: ahornby
Differential Revision: D26513598
fbshipit-source-id: b26cd9a9d89ab0502510b8533df4a60f5ca65292
Summary:
I'm going to start shifting from Thrift to C++ client - to make reuse easier, put the Thrift client in its own module, with the Thrift-specific tests, and put shared tests in a shared module.
While in here, rename the blobstore to ManifoldBlobstore, so that it's clear that it's not just a Thrift blobstore, ready for it being a mix of C++ and Thrift.
Reviewed By: ahornby
Differential Revision: D26513599
fbshipit-source-id: 9e111583f1700eea18e83032b9ca1b9c8babc83e
Summary:
Add support for directly throttling blobstores based on read and write bandwidth used.
Read bytes throttling can only approximate given we only know the size *after* the inner Blobstore::get().
Reviewed By: krallin
Differential Revision: D26022763
fbshipit-source-id: b8019f53bda326117511aa659dcdab2a958dbd27
Summary: Its easier to distinguish the parameters when constructing it directly rather than via a new() method.
Differential Revision: D26017347
fbshipit-source-id: a020db1133de727f217f67a05953059122e3623a
Summary: Allow us to return arg parsing errors rather than panicing
Reviewed By: krallin
Differential Revision: D25837626
fbshipit-source-id: 87e39de140b1dcd3b13a529602fdafc31233175d
Summary:
The correct workflow for using a multi-threaded connection pool for multiple DBs is to have a single shared pool for all the use-cases. The pool is smart enough to maintain separate "pools" for each DB locator and limit them to maximum 100 conn per key.
In this diff I create a `OnceCell` connection pool that is initialized once and reused for every attempt to connect to the DB.
The pool is stored in `MononokeAppData` in order to bind its lifetime to the lifetime of Mononoke app. Then it is passed down as a part of `MysqlOptions`. Unfortunately this makes `MysqlOptions` not copyable, so the diff also contains lots of "clones".
Reviewed By: ahornby
Differential Revision: D25055819
fbshipit-source-id: 21f7d4a89e657fc9f91bf22c56c6a7172fb76ee8
Summary:
benchmark_filestore XDB subcommands uses mysql and has option of using either myrouter or mysql. In this diff I used `args::parse_mysql_options` function to parse the arguments instead of manual processing and get a `MysqlOptions` object.
This is needed later to pass a connection pool object through the `MysqlOptions` struct (see the next diff).
Reviewed By: ahornby
Differential Revision: D25587898
fbshipit-source-id: 66fcfd98ad8f3f9e285ca9635d8f625aa680d7ff
Summary:
Previously needed to pass in cachelib settings once to MononokeAppBuilder and once to parse_and_init_cachelib.
This change adds a MononokeClapApp and MononokeMatches that preserve the settings, thus preventing the need to pass them in twice (and thus avoiding possible inconsistency)
MononokeMatches uses MaybeOwned to hold the inner ArgMatches, which allows us to hold both the usual reference case from get_matches and an owned case for get_matches_from which is used in test cases.
Reviewed By: krallin
Differential Revision: D24788450
fbshipit-source-id: aad5fff2edda305177dcefa4b3a98ab99bc2d811
Summary:
It has a build() method and later in stack it will build a mononoke
specific type rather than the clap::App
Differential Revision: D25216827
fbshipit-source-id: 24a531856405a702e7fecf54d60be1ea3d2aa6e7
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:
This updates the external facing API of the filestore to use 0.3 streams.
Internally, there is still a bit of 0.3 streams, but as of this change, it's
all 0.3 outside.
This required a few changes here and there in places where it was simpler to
just update them to use 0.3 futures instead of `compat()`-ing everything.
Reviewed By: ikostia
Differential Revision: D24731298
fbshipit-source-id: 18a1dc58b27d129970a6aa2d0d23994d5c5de6aa
Summary:
Like it says in the title. This required quite a lot of changes at callsites,
as you'd expect.
Reviewed By: StanislavGlebik
Differential Revision: D24731299
fbshipit-source-id: e58447e88dcc3ba1ab3c951f87f7042e2b03eb2c
Summary: Like it says in the title. This updates `store()` and its (many) callsites.
Reviewed By: ahornby
Differential Revision: D24728658
fbshipit-source-id: 5fccf76d25e58eaf069f3f0cf5a31d2c397687ea
Summary: The more we remove, the easier it'll be to remove the last few problem cases.
Reviewed By: StanislavGlebik
Differential Revision: D24592052
fbshipit-source-id: de6371305991d54bf2802fd904850b49aeb11bbd
Summary:
We want to be able to detect garbage blobs by looking at generation numbers.
Update generation numbers on put, and have a mark command exist to mark blobs as not garbage.
Reviewed By: ahornby
Differential Revision: D23989289
fbshipit-source-id: d96f38649151e3dbd5297cffc262776e74f6cc86
Summary:
Add predicate based PutBehaviour logic to manifoldblob.
This will prevent overwrites of keys when in IfAbsent mode, and will generate useful logging in OverwriteAndLog and IsAbsent mode.
This change factors our part of the put logic to put_check_conflict, so that it can use re-used from each of the PutBehaviour cases.
Reviewed By: StanislavGlebik
Differential Revision: D24021170
fbshipit-source-id: d2e71afadada3d5e661634449108e6c9f8dc5907
Summary: Update Memblob::new callsites to ::default() in preparation for adding arguments to ::new() to specify the put behaviour desired
Differential Revision: D24021173
fbshipit-source-id: 07bf4e6c576ba85c9fa0374d5aac57a533132448
Summary:
This diff introduces Mysql client for Rust to Mononoke as a one more backend in the same row with raw xdb connections and myrouter. So now Mononoke can use new Mysql client connections instead of Myrouter.
To run Mononoke with the new backend, pass `--use-mysql-client` options (conflicts with `--myrouter-port`).
I also added a new target for integration tests, which runs mysql tests using mysql client.
Now to run mysql tests using raw xdb connections, you can use `mononoke/tests/integration:integration-mysql-raw-xdb` and using mysql client `mononoke/tests/integration:integration-mysql`
Reviewed By: ahornby
Differential Revision: D23213228
fbshipit-source-id: c124ccb15747edb17ed94cdad2c6f7703d3bf1a2
Summary:
Generated by formatting with rustfmt 2.0.0-rc.2 and then a second time with fbsource's current rustfmt (1.4.14).
This results in formatting for which rustfmt 1.4 is idempotent but is closer to the style of rustfmt 2.0, reducing the amount of code that will need to change atomically in that upgrade.
---
*Why now?* **:** The 1.x branch is no longer being developed and fixes like https://github.com/rust-lang/rustfmt/issues/4159 (which we need in fbcode) only land to the 2.0 branch.
---
Reviewed By: StanislavGlebik
Differential Revision: D23568780
fbshipit-source-id: b4b4a0aa683d236e2fdeb5b96d723ac2d84b9faf
Summary:
Add a cmdlib argument to control cachelib zstd compression. The default behaviour is unchanged, in that the CachelibBlobstore will attempted compression when putting to the cache if the object is larger than the cachelib max size.
To make the cache behaviour more testable, this change also adds an option to do an eager put to cache without the spawn. The default remains to do a lazy fire and forget put into the cache with tokio::spawn.
The motivation for the change is that when running the walker the compression putting to cachelib can dominate CPU usage for part of the walk, so it's best to turn it off and let those items be uncached as the walker is unlikely to visit them again (it only revisits items that were not fully derived).
Reviewed By: StanislavGlebik
Differential Revision: D22797872
fbshipit-source-id: d05f63811e78597bf3874d7fd0e139b9268cf35d
Summary:
The motivation for making this function async is that it needs to spawn things,
so it should only ever execute while polled by an executor. If we don't do
this, then it can panic if there is no executor, which is annoying.
I've been wanting to do this for a while but hadn't done it because it required
refactoring a lot of things (see the rest of this stack). But, now, it's done.
Reviewed By: mitrandir77
Differential Revision: D21427348
fbshipit-source-id: bad077b90bcf893f38b90e5c470538d2781c51e9
Summary:
Currently, Mononoke's configs are loaded at startup and only refreshed
during restart. There are some exceptions to this, including throttling limits.
Other Mononoke services (such as the LFS server) have their own implementations
of hot reloadable configs, however there isn't a universally agreed upon method.
Static configs makes it hard to roll out features gradually and safely. If a
bad config option is enabled, it can't be rectified until the entire tier is
restarted. However, Mononoke's code is structured with static configs in mind
and doesn't support hot reloading. Changing this would require a lot of work
(imagine trying to swap out blobstore configs during run time) and wouldn't
necessarily provide much benefit.
Instead, add a subset of hot reloadable configs called tunables. Tunables are
accessible from anywhere in the code and are cheap to read as they only require
reading an atomic value. This means that they can be used even in hot code
paths.
Currently tunables support reloading boolean values and i64s. In the future,
I'd like to expand tunables to include more functionality, such as a rollout
percentage.
The `--tunables-config` flag points to a configerator spec that exports a
Tunables thrift struct. This allows differents tiers and Mononoke services to
have their own tunables. If this isn't provided, `MononokeTunables::default()`
will be used.
This diff adds a proc_macro that will generate the relevant `get` and `update`
methods for the fields added to a struct which derives `Tunables`. This struct is
then stored in a `once_cell` and can be accessed using `tunables::tunables()`.
To add a new tunable, add a field to the `MononokeTunables` struct that is of
type `AtomicBool` or `AtomicI64`. Update the relevant tunables configerator
config to include your new field, with the exact same name.
Removing a tunable from `MononokeTunables` is fine, as is removing a tunable
from configerator.
If the `--tunables-config` path isn't passed, then a default tunables config
located at `scm/mononoke/tunables/default` will be loaded. There is also the
`--disable-tunables` flag that won't load anything from configerator, it
will instead use the `Tunable` struct's `default()` method to initialise it.
This is useful in integration tests.
Reviewed By: StanislavGlebik
Differential Revision: D21177252
fbshipit-source-id: 02a93c1ceee99066019b23d81ea308e4c565d371
Summary:
This bitrot with two different changes:
- D19473960 put it on a v2 runtime, but the I/O is v1 so that doesn't work (it
panics).
- The clap update a couple months ago made duplicate arguments illegal, and a
month before that we had put `debug` in the logger args (arguably where it
belong), so this binary was now setting `debug` twice, which would now panic.
Evidently, just static typing wasn't quite enough to keep this working through
time (though that's perhaps due to the fact that both of those changes were
invisible to the type system), so I also added a smoke test for this.
Reviewed By: farnz
Differential Revision: D20618785
fbshipit-source-id: a1bf33783885c1bb2fe99d3746d1b73853bcdf38
Summary:
This shifts the responsibility of mocking all facebook-specific code
to mononoke's sql_ext crate. If OSS code calls into any of that code it will
most likely result in a panic.
Reviewed By: ahornby
Differential Revision: D20247580
fbshipit-source-id: 43f158d91aa32adaa5df6e3786243fb89c9ce961
Summary:
At this point all future-returning `fn`s in `benchmark_filestore`
are async.
Reviewed By: krallin
Differential Revision: D20615922
fbshipit-source-id: 8b9998e4d8933e46f0c2612270faefcb061eae37
Summary:
This diff is a combination of an asyncification and a refactor:
- isolatedthe logic for setting `blob` (which gets overwritten
5 times, often blocking on futures in the process) into it's own
function `get_blob`, which I think makes it a lot easier to see
which variables depend on one another.
- push all the rest of the logic into an async function
`run_benchmark_filestore`
Reviewed By: krallin
Differential Revision: D20615743
fbshipit-source-id: 9d0918e4078b8193c59758701c5dc5e744298d1d
Summary:
This is a very small struct (2 u64s) that really doesn't need to be passed by
reference. Might as well just pass it by value.
Differential Revision: D20281936
fbshipit-source-id: 2cc64c8ab6e99ee50b2e493eff61ea34d6eb54c1
Summary: separate out the Facebook-specific pieces of the sql_ext crate
Reviewed By: ahornby
Differential Revision: D20218219
fbshipit-source-id: e933c7402b31fcd5c4af78d5e70adafd67e91ecd
