Summary:
Cleanup Rust APIs and underlying Cxx FFI helpers to be more correct
- most u32 values are not valid ports (u16), end-user API must make it hard/impossible to pass garbage values
- fix other parameters that were i32 and should have been u32/u64 (+ reliance on implicit conversions on the Cxx side)
- update all users + sfcli Rust template
Reviewed By: dtolnay
Differential Revision: D28395107
fbshipit-source-id: 699253b04cfa03e3684bbd253116406fc658ed97
Summary:
Currently, we take a `&mut (dyn Buf + Send)` as input when writng to Manifold.
This has a couple of downsides:
- It's not very ergonomic. From the API it's not obvious what mutations are
going to be done on your `Buf` exactly (it's going to be consumed entirely),
and it often results in code that's a little clumsy (see what I had to change
here), where make calls like `write(&key, &mut mydata.clone())`.
- It limits what we can do with it. If you already have a `IOBufShared` on
hand, you shouldn't need to copy it in order to pass it to Manifold, but
currently the Manifold client code does have to copy it because all it sees
is a `dyn Buf`.
This diff updates the client to take a `IOBufShared` directly, which has plenty
of convenient `From<...>` conversions (and some efficient ones like
`From<Bytes>`, which doesn't copy the `Bytes` at all).
Reviewed By: ahornby, Imxset21
Differential Revision: D28535539
fbshipit-source-id: bba1b963a96350ad57cc0fbfcc31f7e1eb36c317
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:
Paying the setup and teardown overhead of multiple processes seems silly, when we can pack in parallel in a single process.
Make it possible to run multiple packing runs from a single packer process
Reviewed By: ahornby
Differential Revision: D28508527
fbshipit-source-id: eab07d028db46d62731f06effbde2f5bc5579000
Summary:
Improve git-import speed by grouping many BonsaiChangests into each call to save_bonsai_changesets.
During earlier profiling noticed that the speed of gitimport was dictated by the save_bonsai_changesets, by doing this we can split the speed more evenly between save_bonsai_changesets and the steps to derive the diff and upload the missing file-blobs.
For profiling and performance analysis details please see
https://fb.workplace.com/groups/1619247701796399/permalink/1709890502732118/
Reviewed By: StanislavGlebik
Differential Revision: D28497973
fbshipit-source-id: 0fbcf37535554dd96664da4906633eeb07c58f7c
Summary:
In D28287486 (b9b5e16dcf) we added logic that should prevent some of the failures on
is_present() checks. However some parts of the codebase rely on `get()`
returning None and they don't use is_present() at all - in particular, that's
what configerator-a is doing.
Given that we have failures of our derived data tailer, let's try to replicate
the logic from is_present() to get().
Reviewed By: krallin
Differential Revision: D28506739
fbshipit-source-id: a37d6c4b5a43aa9a1284499831fcdc3ee5605b9e
Summary:
This is currently calling `Buf::bytes()` in order to get a `&[u8]` out of
`Bytes`, but this method isn't actually guaranteed to return all data in the
`Buf`: https://docs.rs/bytes/0.5.6/bytes/trait.Buf.html#tymethod.bytes
> Note that this can return shorter slice (this allows non-continuous internal representation)
In practice this is fine because `Bytes` does in fact have a contiguous
internal representation (which is why we can call `as_ref()` on it), but let's
do the more correct thing here.
(I happened to spot this while looking into the feasibility of a Bytes 0.5 ->
Bytes 1.x upgrade, where this `bytes()` method was renamed to `chunk()`)
Reviewed By: farnz
Differential Revision: D28507885
fbshipit-source-id: 73e5f1ba587292f772c702127a3933ea76fceb9f
Summary:
This test is a bit flaky because its execution depends on the other in which 2
futures that are spawned execute, which isn't entirely deterministic (those
tests were using a multi-threaded runtime, I'm also fixing this here).
Reviewed By: ahornby
Differential Revision: D28507337
fbshipit-source-id: 3c33dc7ffe73de0c6696523ed49d3b30ceda82c0
Summary:
Simple test that syncs a single commit from a source to a target.
A few notes
1) I want to use test Mononoke instance, but I can't initialize them in another
crate because of cfg(test). I removed cfg(test) to make it possible.
2) I initialized megarepo mapping in TestRepoFactory so that all tables
were in the same db. It's not strictly necessary, but makes initialization a bit simpler.
Reviewed By: mitrandir77
Differential Revision: D28444865
fbshipit-source-id: f39c917806709606ce8e5a1c1158f186d239d8b8
Summary:
In order to sync changesets we need to know what was the latest synced
changeset from a given source. Previously we wanted to store this data in sql,
however later we decided to store it in the file in the repo. The main reason
for doing that is that storing data inside the repo gives us both blame and
history, while database can't provide that.
This diff implements that - removes all the sql-related code for storing latest
synced changesets and instead puts the data inside the file in the repository.
Note that we still using sql to store mapping between (source, target)
changesets.
Reviewed By: mitrandir77
Differential Revision: D28470102
fbshipit-source-id: 996905b17ad4b0d5b0ea1e40c73d762850e113a8
Summary: Now it's at least possible to add new configs
Reviewed By: mitrandir77
Differential Revision: D28444866
fbshipit-source-id: cf0e2e737a125bdbd3b7eff55e8ee3f1d5a193d2
Summary:
Like it says in the title, this updates bookmarks to track the log id of their last update.
This will let us do incremental queries from the WBC. See the design doc linked to the
task here for details.
Reviewed By: farnz
Differential Revision: D28444852
fbshipit-source-id: e966ced8e136ad6c4306e977e21670caa457ea71
Summary:
Like it says in the title. This updates various streams so that they yield
after 10+ ms on CPU, thus ensuring that they don't starve other tasks of
runtime.
Reviewed By: johansglock
Differential Revision: D28441879
fbshipit-source-id: 78a8cdaefbe37d4768211dfa8a87c045ed898d57
Summary: The new and new_with_ttl constructors were causing duplication. Combine them.
Reviewed By: farnz
Differential Revision: D28471162
fbshipit-source-id: c7095a9a337d0ccbf5cd15ac3650cd5b361aaebf
Summary:
manual_scrub was using the ordered form of buffered so that checkpoint was written correctly.
This diff switches to buffered_unordered which can give better throughput. To do so checkpoint uses a tracker to know what keys have completed, so it can save the latest done key which has no preceding keys still executing.
Reviewed By: farnz
Differential Revision: D28438371
fbshipit-source-id: 274aa371a0c33d37d0dc7779b04daec2b5e1bc15
Summary: There isn't much point to having a trivial setter method for a public field.
Reviewed By: krallin
Differential Revision: D28427464
fbshipit-source-id: e0800fae6f635612e72945e570ce4de692dfb7bb
Summary: Let's verify what we know can go wrong.
Reviewed By: StanislavGlebik
Differential Revision: D28419082
fbshipit-source-id: f1f50de7bff4cf3dc1469ecfbd3744624f26c08d
Summary:
This adds the ability to spawn sync_changeset() request using scs. We don't
have async reqeusts implementation, so it's quite hacky - it doesn't return the
actual synced changeset id. we'll add it later once async queue is implemneted
Reviewed By: ikostia
Differential Revision: D28441052
fbshipit-source-id: 5dd32c1638246442c6a9b1e21d7a8d73bfe762ab
Summary:
One more attempt to mitigate S231752.
From the debugging we did locally it seems that warning about expensive
getbundle might prevent keep alives from being send to the client.
adding yield seem to mitigate the issue locally.
We are not 100% sure it will actually mitigate the sev, but it seems like a
cheap and safe thing to try.
Reviewed By: krallin
Differential Revision: D28438570
fbshipit-source-id: 14f613222e8f69c4cbe490d8fa608d79c30b1a4d
Summary: This is useful to get an idea of what the scrub is doing
Reviewed By: farnz
Differential Revision: D28417785
fbshipit-source-id: 1421e0aae13f43371d4c0d066c08aee80b17e9c0
Summary:
During S231236 the LFS servers were using a lot of CPU and were
compressing blobs sent to clients. Thomas hotfixed the servers to disable
compression in order to save some CPU. Instead of having to rebuild and
redeploy the LFS server, update it to be able to disable compression using
configerator.
The `disable_compression` option will disable compression of downloads
globally. The `disable_compression_identities` allows us to disable compression
for some group of identities.
I also refactored some of the shared code from `download` and `download_sha256`
into a new function, `download_inner`.
Reviewed By: krallin
Differential Revision: D28382405
fbshipit-source-id: 792f10a9e3bb32b56ef87aa8e0b2c4b098567579
Summary: Write mostly stores are often in the process of being populated. Add an option to control whether scrub errors are raised for missing values in write mostly stores.
Differential Revision: D28393689
fbshipit-source-id: dfc371dcc3b591beadead82608a747958b53f580
Summary:
Couple of small changes I originally did during next diff split out on their own.
Save a loop over the scrub stores if there are relevant entries on the queue and remove unnecessary repetition of type.
Reviewed By: farnz
Differential Revision: D28411617
fbshipit-source-id: 2015b5b1d68f870b09fbd8929a59c21fe4f57c87
Summary:
When you spawn a task, Tokio puts it on a "LIFO slot" associated with the
current thread. While the task is in the LIFO slot, it is not eligible to be
run by other threads.
If the thread that just spawned `fwd` above goes and does some expensive
synchronous work in `request_handler` (we'd like to avoid that but sometimes
that happens), then that will delay `fwd`.
This means that notably keepalives will not be sent (you can repro by putting a
`std:🧵:sleep` right after we spawn `fwd`). To mitigate this, we spawn
another dummy taks here. This task will take `fwd`'s place in the LIFO slot,
thus pushing `fwd` onto a task queue where other runtime threads can claim it.
This way, even if this thread goes do some expensive CPU-bound work, we won't
delay keepalives.
This doesn't solve the fact that we probably shouldn't be doing such CPU-bound
work to begin with, but at least it might mitigate the issue such that we don't
have I/O delayed even though the runtime isn't visibly overloaded.
Reviewed By: johansglock
Differential Revision: D28412114
fbshipit-source-id: b56c0156ac6cf991cc899a82e3d2a96c63216fda
Summary: Add a write mostly store to the scrub test cases. This in preparation for next diff where the write mostly case has a new scrub option added.
Reviewed By: krallin
Differential Revision: D28393690
fbshipit-source-id: f7878f5e25814a7d327b1a80d4f96c0867944a14
Summary: Log the blobstore stack being used for the scrub
Reviewed By: farnz
Differential Revision: D28408340
fbshipit-source-id: 2299f7f7397f48d70b9a8295f0aa28c89bbf5809
Summary: These will be used in the next diffs, let's add initialization here.
Reviewed By: ikostia
Differential Revision: D28409734
fbshipit-source-id: f656db8259f28559df52562f0590382d89f1f0c0
Summary:
This is an attempt to split D28357210 to make it a bit smaller, so this
new function will be used later.
Reviewed By: krallin
Differential Revision: D28386004
fbshipit-source-id: 42c9ead0668fb78747dcb0fc7c89b6f181f7d9e6
Summary:
We have clients mysteriously timing out despite the fact that we should
normally be sending keepalives. To understand this, add some logging so that if
a client disconnects, we log a lot more detailed information to Scuba.
Reviewed By: mitrandir77
Differential Revision: D28383122
fbshipit-source-id: 0e49262bcc08c75f8e06eae17c882742b58ea51d
Summary:
I'd like to have some socket level logging, and it's good to capture the
metadata there so that we have client identities, session ids, etc.
But I can't do this if I don't have the metadata! This updates our code so that the
metadata is available where this logging will go.
This means we can't rely on loggers in `try_convert_preamble_to_metadata`, but
given this is now only used for hgcli, I don't think that matters, so I just removed it.
Reviewed By: mitrandir77
Differential Revision: D28383121
fbshipit-source-id: 27532f021a9082e74403bba73cad4fc6d0a6d97c
Summary:
I'd like to have some socket level logging, and it's good to capture the
metadata there so that we have client identities, session ids, etc.
To do so I need to not move it into the session. This does that.
Reviewed By: mitrandir77
Differential Revision: D28383123
fbshipit-source-id: 3fd10c3720465824dbcb2528227cbb3521d3068a
Summary:
This struct is intended to be a single entry-point to the megarepo logic. It is also intended to be owned directly by scs_server without the `Mononoke` struct (from `mononoke_api`) intermediary. In effect, this means that mononoke server won't be affected by `MegarepoApi` at all.
Apart from adding this struct, this diff also adds instantiation of prod `AsyncMethodRequestQueue` and wires it up to the scs_server to enqueue and poll requests.
Reviewed By: StanislavGlebik
Differential Revision: D28356563
fbshipit-source-id: b67ee48387d794fd333d106d3ffd40124086c97e
Summary:
With `MegarepoApi` struct in play, there is a genuine need to have two repo
factories in a single process: this allows the structure to be self-sufficient
and instantiated without any references to `Mononoke` from `monooke_api`.
While this need could be solved by just wrapping a `RepoFactory` in an `Arc`,
it seems like most of it is already clonable, so let's just make it fully
clonable by fixing a few remaining places. (I prefer this over `Arc`, because
there's less refactoring of unrelated code). Given that there will likely be a
single digit of repo factories instantiated in a single process, the difference
between a single arc's clone (`Arc<RepoFactory>`) and ~10 arc clones (what I
did) is negligible.
Differential Revision: D28379860
fbshipit-source-id: fbddbdc913fedcd5846366344bc2f2c1ec4bd91e
Summary: This way implementing MegarepoApi is more convenient.
Reviewed By: krallin
Differential Revision: D28355487
fbshipit-source-id: e7643e854ee46fe6cb9c4a882f6c677bf4e77262
Summary:
Partial getbundle optimization didn't work correctly if one merge parent was an ancestor
of another - it might return a parent commit before a child commit. Say we had a graph like this
```
C
| \
| B
| /
A
```
Previously partial getbundle optimization could have visited A before it
visited B, and returned commits in the wrong order, which in turn would lead to
"hg pull" failures. The reason for that is that we don't order commits by generation number.
This diff fixes it by using UniqueHeap to sort commits by generation number
before returning them. Also note that as the name suggests UniqueHeap stores
only unique values in the heap, so we don't need to keep separate `visited`
hashset
Reviewed By: krallin
Differential Revision: D28378145
fbshipit-source-id: 9467fb7cfa8386e9e2725b80f386240b522ff3ee
Summary:
Just as with rewrite_commit function that I moved a to commit_transformation
not so long ago (D28259214 (df340221a0)), let's also move copy_file_contents. The motivation
is because we are going to use it in the next diff for sync_changeset method.
Reviewed By: ikostia
Differential Revision: D28352402
fbshipit-source-id: 12288a51540c9793d988e4063735bcbc1c3b1a7f
Summary:
Those blobs are designed to fit in cachelib, so we shouldn't attempt to
zstd-encoded them (not to mention that they don't usually compress very well
since many of those blobs come from binary files, though that's not true of
all).
However, we weren't actually doing this right now. There were a few reasons:
- Our threshold didn't allow enough headroom. I don't know when exactly this
got introduced (of indeed if that has worked since we introduced cachelib
compression).
- We serialize a bunch of extra metadata that we really don't need as it's a
bit meaningless once it's gone through the cache (we notably don't serialize
this in Memcache). This diff updates us to just store bytes here.
Differential Revision: D28350387
fbshipit-source-id: 4d684ab58cea137044e20951ec4cbb21240b8dfc
Summary: See the next diff for motivation: this makes it easier to implement.
Differential Revision: D28350388
fbshipit-source-id: 026605cf8296a945d6cc81b7f36d9198325bf13c
Summary:
Test is flaky: https://www.internalfb.com/intern/test/281474993296146?ref_report_id=0
I suppose this happens due to mechanics how we measure qps - with low volume, some averaging or bucketing might not work as precisely as with a lot of qps we have in normal, prod scenarios.
Lowering the threshold by 1 should fix this.
Reviewed By: ahornby
Differential Revision: D28350150
fbshipit-source-id: 694bfb8cce1935704e35b27f7d4455439d4bfffe
Summary:
I should've made them structs from the beginning, but of course I thought that
I know better and these tokens can definitely not be richer than just strings.
Well, it turns out we need them to be richer. Specific reason is that in theory
a single Mononoke (or scs_server) instance can run with multiple storage
configs. For us this means that one target's requests may be stored in one
db, while another target's requests - in another one. For blobstores this is
even used in practice, while for xdb it's just a theoretical thing, but we need
to support it nevertheless.
To do so, let's add the ability to query the target (and, correspondingly, the
Mononoke repo) from any king of params our async methods receive: ThriftParams
or Token implementors.
In addition, this diff really implements `AddScubaParams` and `AddScubaResponse` for more things than before, so there's that.
Finally, apart from making tokens structured, this diff also changes an interface in two more ways:
- it adds optional `message` fields to several params structs
- it adds `changesets_to_merge` to `MegarepoChangeTargetConfigParams`
Reviewed By: StanislavGlebik
Differential Revision: D28333999
fbshipit-source-id: 99bd19b040b59ee788ef661dda3171cc56254d33
Summary: This is going to be used in the next diffs.
Reviewed By: StanislavGlebik
Differential Revision: D28333977
fbshipit-source-id: ad52d307e13ae9bd662209ef7ec6afdcf0ee24c7
Summary:
This has been here for a little while, but it's worth changing. Currently, we
entirely discard logs coming via a CoreContext in EdenAPI.
We don't typically log many of those anywhere in Mononoke, but when we do they
tend to be for error conditions, warnings, or aggregated reporting, and can be
quite meaningful as a result.
So, let's update to not discard them. To make it easy to differentiate those
logs from EdenAPI request-level logs (even though they'll both have `service` =
`edenapi`), let's give the latter a Log Tag (which is consistent with what
we do in repo client).
Differential Revision: D28350733
fbshipit-source-id: 3b12a4b56f28435460186e1f7578163ca7bdaebc
Summary:
Previously it was possible to write configs only, now it's possible to read
them as well.
Reviewed By: ikostia
Differential Revision: D28326571
fbshipit-source-id: d946201a384cc3998d1c197b7eabb77b9f35129d
Summary:
Adding mappng to keep track of two things:
1) keep track of the latest source commit that was synced into a given target - this will be used during sync_changeset() method to validate if a parent changeset of a given changeset was already synced
2) which source commit maps to what target commit
Reviewed By: ikostia
Differential Revision: D28319908
fbshipit-source-id: f776d294d779695e99d644bf5f0a5a331272cc14
Summary:
This is going to be use to rewrite (or transform) commits from source to
target. This diff does a few tihngs:
1) adds a MultiMover type and a function that produces a mover given a config. This is similar to Mover type we used for fbsource<-> ovrsource megarepo sync, though this time it can produce a few target paths for a given source path.
2) Moves `rewrite_commit` function from cross_repo_sync to megarepo_api, and make it work with MultiMover.
Reviewed By: ikostia
Differential Revision: D28259214
fbshipit-source-id: 16ba106dc0c65cb606df10c1a210578621c62367
Summary:
This crate is a foundation for the async requests support in megarepo service.
The idea is to be able to store serialized parameters in the blobstore upon
request arrival, and to be able to query request results from the blobstore
while polling.
This diff manipulates the following classes of types:
- param types for async methods: self-explanatory
- response types: these contain only a resulting value of a completed successful execution
- stored result types: these contain a result value of a completed execution. It may either be successful or failed. These types exist for the purpose of preserving execution result in the blobstore.
- poll-response types: these contain and option of a response. If the optional value is empty, this means that the request is not yet ready
- polling tokens: these are used by the client to ask about the processing status for a submitted request
Apart from that, some of these types have both Rust and Thrift counterparts, mainly for the purposes of us being able to implement traits for Rust types.
Relationships between these types are encoded in various traits and their associated types.
The lifecycle of an async request is as follows therefore:
1. the request is submitted by the client, and enqueued
1. params are serialized and saved into a blobstore
1. an entry is created in the SQL table
1. the key from that table is used to create a polling token
1. some external system processes a request [completely absent form this diff]
1. it notices a new entry in the queue
1. it reads request's params from the blobstore
1. it processes the request
1. it preserves either a success of a failure of the request into the blobstore
1. it updates the SQL table to mention that the request is now ready to be polled
1. the client polls the request
1. queue struct receives a polling token
1. out of that token it constructs DB keys
1. it looks up the request row and checks if it is in the ready state
1. if that is the case, it reads the result_blobstore_key value and fetches serialized result object
1. now it has to turn this serialized result into a poll response:
1. if the result is absent, poll response is a success with an empty payload
1. if the result is present and successful, poll response is a success with the result's successful variant as a payload
1. if the result is present and is a failure, the polling call throws a thrift exception with that failure
Note: Why is there yet another .thrift file introduced in this diff? I felt like these types aren't a part of the scs interface, so they don't belong in `source_control.thrift`. On the other hand, they wrap things defined in `source_control.thrift,` so I needed to include it.
Reviewed By: StanislavGlebik
Differential Revision: D27964822
fbshipit-source-id: fc1a33a799d01c908bbe18a5394eba034b780174
