Summary: This is more flexible.
Reviewed By: StanislavGlebik
Differential Revision: D26168559
fbshipit-source-id: 5946b8b06b3a577f1a8398a228467925f748acf7
Summary: Just as we have global strings/ints, let's have per-repo ones.
Reviewed By: StanislavGlebik
Differential Revision: D26168541
fbshipit-source-id: f31cb4d556231d8f13f7d7dd521086497d52288b
Summary:
Please see added test. Without this diff such test does not even compile,
as `new_values_by_repo` is moved out by `self.#names.swap(Arc::new(new_values_by_repo));` after processing the first tunable (line 202).
Reviewed By: StanislavGlebik
Differential Revision: D26168371
fbshipit-source-id: 3cd9d77b72554eb97927662bc631611fa91eaecb
Summary:
Yesterday we had an alarm when blobstore sync queue got overloaded again. This
time it was caused by a large commit cloud commit landing and writing lots of
content and alias blobs.
As we discussed before, let's add an option that would allow us to not write to
blobstore sync queue for commit cloud pushes of content and aliases.
It would slightly increase the latency, but will protect blobstore sync queue
from overloading.
Reviewed By: farnz
Differential Revision: D26129038
fbshipit-source-id: 0e96887e3aa3cf26880899c820f556bb16c437cb
Summary:
Lots of generated code in this diff. Only code change was in
`common/rust/cargo_from_buck/lib/cargo_generator.py`.
Path/git-only dependencies (ie `mydep = { path = "../foo/bar" }`) are not
publishable to crates.io. However, we are allowed to specify both a path/git
_and_ a version. When building locally, the path/git is chosen. When publishing,
the version on crates.io is chosen.
See https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations .
Note that I understand that not all autocargo projects are published on crates.io (yet).
The point of this diff is to allow projects to slowly start getting uploaded.
The end goal is autocargo generated `Cargo.toml`s that can be `cargo publish`ed
without further modification.
Reviewed By: lukaspiatkowski
Differential Revision: D26028982
fbshipit-source-id: f7b4c9d4f4dd004727202bd98ab10e201a21e88c
Summary:
A bit of background: some time ago I landed an optimization to getbundle, which
split the commits user want to fetch into "high generation numbers"
(which means commits are close to main bookmark) and "low generation numbers"
(commits that are far away from the main bookmark). User can fetch "low
generation numbers" if e.g. a commit to an old release branch was landed.
Processing them separately can yield significant speedups.
Previously we chose a threshold statically e.g. if a commit has generation
number lower than X then it's considered to have a low generation number.
Threshold was chosen arbitrarily and relatively small [1], and it didn't work
that well for commits that were above this threshold, but still not close
enough to main bookmark - we still see cpu spikes.
Instead let's define a commit as having low generation number if it's >X
commits farther from the commit with the highest generation number.
Reviewed By: ahornby
Differential Revision: D25995936
fbshipit-source-id: 57eba4ba288114f430722266a8326183b3b1f0bd
Summary:
Yesterday I landed a diff (D25950531 (79c34c5094)) which allows changing a mapping by
landing a commit with a special commit extra. The implementation was done
inside backsyncer.
However, this was incorrect for a few reasons, the main reason being backsyncer
is not the only thing that rewrites commits between repos (x-repo commit lookup
is another thing for example).
So we need to push this implementation down to CommitSyncer level so that
everyone rewrites commits in the same way.
This diff fixes this issue, and moves all the logic of figuring out correct
mapping version down to CommitSyncer level. Doing so allowed me to also
simplify backsyncer to use normal sync_commit() method to backsync commits.
Another important note is about how we handle commits with no parents. We allow
syncing them only if there's an unambiguous version that we can pick. In case
of any ambiguity we don't sync them and return an error, which means that e.g.
merging a new repo and simultaneously changing the mapping is not possible.
Reviewed By: ikostia
Differential Revision: D25975842
fbshipit-source-id: a87fee545ac1305832ac905337610e7b87884477
Summary:
At the moment the procedure to change commit sync mapping version is fairly
complicated [1]. This diff attempts to simplify one aspect of it.
At the moment we need to disable pushredirection and manually sync a commit
between two repos. Disabling pushredirection is quite error prone and led to
SEVs in the past. With this diff we just need to create a commit with a special
extra "change-xrepo-mapping" and let the backsyncer rewrite a commit with the
new version for us.
This means that landing a commit with this extra should not be allowed to
everyone - I'm going to change bookmark movement to check that.
Reviewed By: ikostia
Differential Revision: D25950531
fbshipit-source-id: 78b1f4950e6bb7da8b511e02685ed6084c29a680
Summary:
We had issues with mononoke writing too much blobstore sync queue entries while
deriving data for large commits. We've contemplated a few solutions, and
decided to give this one a go.
This approach forces derive data to use Background SessionClass which has an
effect of not writing data to blobstore sync queue if the write to blobstore
was successful (it still writes data to the queue otherwise). This should
reduce the number of entries we write to the blobstore sync queue
significantly. The downside is that writes might get a bit slower - our
assumption is that this slowdown is acceptable. If that's not the case we can
always disable this option.
This diff overrides SessionClass for normal ::derive() method. However there's
also batch_derive() - this one will be addressed in the next diff.
One thing to note - we still write derived data mapping to blobstore sync queue. That should be find as we have a constant number of writes per commits.
Reviewed By: krallin
Differential Revision: D25910464
fbshipit-source-id: 4113d00bc0efe560fd14a5d4319b743d0a100dfa
Summary:
The goal of this stack is to start logging commits to scribe even if a commit was
introduced by scs create_commit/move_bookmark api. Currently we don't do that.
Initially I had bigger plans and I wanted to log to scribe only from bookmarks_movement and remove scribe logging from unbundle/processing.rs, but it turned out to be trickier to implement. In general, the approach we use right now where in order to log to scribe we need to put `log_commit_to_scribe` call in all the places that can possibly create commits/move bookmarks seems wrong, but changing it is a bit hard. So for now I decided to solve the main problem we have, which is the fact that we don't get scribe logs from repos where bookmarks is moved via scs methods.
To fix that I added an additional option to CreateBookmark/UpdateBookmark structs. If this option is set to true then before moving/creating the bookmark it finds all draft commits that are going to be made public by the bookmark move i.e. all draft ancestors of new bookmark destinationl. This is unfortunate that we have to do this traversal on the critical path of the move_bookmark call, but in practice I hope it won't be too bad since we do similar traversal to record bonsai<->git mappings. In case my hopes are wrong we have scuba logging which should make it clear that this is an expensive operation and also we have a tunable to disable this behavioiur.
Also note that we don't use PushParams::commit_scribe_category. This is intentional - PushParams::commit_scribe_category doesn't seem useful at all, and I plan to deprecate it later in the stack. Even later it would make sense to deprecate PushrebaseParams::commit_scribe_category as well, and put commit_scribe_category optoin in another place in the config.
Reviewed By: markbt
Differential Revision: D25558248
fbshipit-source-id: f7dedea8d6f72ad40c006693d4f1a265977f843f
Summary:
The backfiller may read or write to the blobstore too quickly. Apply QPS
limits to the backfill batch context to keep the read or write rate acceptable.
Reviewed By: ahornby
Differential Revision: D25371966
fbshipit-source-id: 276bf2dd428f7f66f7472aabd9e943eec5733afe
Summary:
This adds support for optionally not uploading commits we already have when
they arrive via infinitepush. This can happen if we're replaying bundles.
This works by filtering the commits we have. We still get some futures created
for the underlying uploads, but we never poll them because when we visit
what futures are needed for what commits, we don't select uploads that are
only reachable from commits we filtered out.
Obviously, this isn't super efficient, since the client still has to send us
all this data, but it's better than having them send us all that data then
having us take hours overwriting it all.
Reviewed By: mitrandir77
Differential Revision: D25120844
fbshipit-source-id: f96881d0d98ec622259bedb624fd94b600a2bf1d
Summary:
Move the check for commits not having case conflicts from upload time to when
the commit is being landed to a public bookmark.
This allows draft commits in commit cloud to contain erroneously-introduced
case conflicts, whilst still protecting public commits from these case conflicts.
Note that the checks we are moving are the checks for whether this commit
contains case conflicts internally (two files added by this commit conflict in
case), or against the content of ancestor commits (the commit adds a file which
conflicts with an existing file). The check for whether this commit and the
commits it is being pushrebased over conflict still happens during pushrebase.
This behaviour is controlled by a pair of tunable flags:
* `check_case_conflicts_on_bookmark_movement` enables the new check at land time
* `skip_case_conflict_check_on_changeset_upload` disables the old check at upload-time
The `check_case_conflicts_on_bookmark_movement` should always be set if the
`skip_case_conflict_check_on_changeset_upload` is set, otherwise users will
be able to land case conflicts. Setting `check_case_conflicts_on_bookmark_movement`
without setting `skip_case_conflict_check_on_changeset_upload` allows both
checks to be enabled if that is necessary.
To handle bookmark-only moves, the `run_hooks_on_additional_changesets` tunable
flag must also be enabled.
Reviewed By: ahornby
Differential Revision: D24990174
fbshipit-source-id: 34e40e389f2c2139ba24ecee75473c362f365864
Summary:
Previously it was a config knob, but they are rather hard to change because
they require a restart of the service. Let's make it a tunable instead.
Reviewed By: farnz
Differential Revision: D24682129
fbshipit-source-id: 9832927a97b1ff9da49c69755c3fbdc1871b3c5d
Summary:
This adds some basic Scuba logging to commit sync logic in Mononoke. This can be disabled via a tunable.
Note: of course, the wrapping of logging functions would have been pretty cool to do in a macro, but I don't have time atm to figure it out, so have just code.
Reviewed By: StanislavGlebik
Differential Revision: D24571702
fbshipit-source-id: f8830fc675a2a62d2204671e86ab2c819372c5cc
Summary:
Some requests can result in a large number of blob fetches. Add rate limiting
so that these requests don't use up all available capacity.
Rate limits can be specified in tunables.
Reviewed By: ahornby
Differential Revision: D24592814
fbshipit-source-id: 9a73a92094d0ce01be5491b65b4808e3ebb05c11
Summary:
We don't want to log slow derivations in tests. To do that, let's look at at
unable to decide if we should log.
We only log if the threshold is > 0, so since the default is 0, that means we
won't log unless a config overrides it (which won't be the case in tests, but
will be the case everywhere else: D24329372).
Reviewed By: StanislavGlebik
Differential Revision: D24329377
fbshipit-source-id: e9d70236c614780e383870297d08afaf64450383
Summary:
Let's make this configurable so we can control how many fallbacks we want to
allow if we're overloaded.
Reviewed By: farnz
Differential Revision: D24017088
fbshipit-source-id: 9bccaf831a28daff9696950ae8aac9d53e0c51c0
Summary:
D23599866 (54d43b7f95) added an optimization for getbundle that reduces cpu usage when a new
commit with log generation number is added. I.e. the case like this
```
O
|
O
..
O <- new commit, low generation number
|
...
```
Unfortunately this optimization doesn't help with the case where a new repo is
merged into master
```
O <- also new commit, but generationo number is high!
| \
.. O <- new commit, low generation number, but it's not in "heads" parameter
|
|
O
...
```
The merge commit actually has a high generation number, but it's p2 has a low
generation number, so it causes the same issue with high cpu usage.
This diff adds a second optimization ( :( ) that should help with the shortcoming of the first one. See comments for more details.
Reviewed By: ikostia
Differential Revision: D23824204
fbshipit-source-id: 8f647f1813d2662e41325829d05def633372c140
Summary:
There were a few instances of timed out getpack requests on ovrsource.
Example: https://fburl.com/sandcastle/yylr1w3v
Let's bump the timeout to unblock them.
Reviewed By: krallin
Differential Revision: D23900374
fbshipit-source-id: 3ee6e2d4f6b6ed12cd0c1516c686a03c87fa7cb4
Summary:
Currently getbundle implementation works this way: it accepts two list of
changesets, a list of `heads` and a list of `common`. getbundle needs to return
enough changesets to the client so that client has `heads` and all of its
ancestors. `common` is a hint to the server that tells which nodes client
already has and doesn't need sending.
Current getbundle implementation is inefficient when any of the heads have a low
generation number but also we have a lot excludes which have a high generation
number. Current implementation needs to find ancestors of excludes that are
equal or lower than generation of this head with a low generation number.
This diff is hacky attempt to improve it. It does it by splitting the heads
into heads with high generation numbers and heads with low generation numbers,
sending separate getbundle requests for each of them and then combining the
results.
Example
```
O <- head 1
|
O <- exclude 1
|
... O <- head 2 <- low generation number
| |
O O <- exclude 2
```
If we have a request like `(heads: [head1, head2], excludes: [exclude1, exclude2])` then this optimization splits it into two requests: `(heads: [head1], excludes: [exclude1, exclude2] )` and `(heads:[head2], excludes:[exclude2])`, and then combines the results. Note that it might result in overfetching i.e. returning much more commits to the client then it requested. This might make a request much slower, and to combat that I suggest to have a tunable getbundle_low_gen_optimization_max_traversal_limit that prevents overfetching.
Reviewed By: krallin
Differential Revision: D23599866
fbshipit-source-id: fcd773eb6a0fb4e8d2128b819f7a13595aca65fa
Summary:
Previously we were not logging a redacted access if previous access was logged
less < MIN_REPORT_TIME_DIFFERENCE_NS ago. That doesn't work well with our
tests.
Let's instead add a sampling tunable.
Reviewed By: krallin
Differential Revision: D23595067
fbshipit-source-id: 47f6152945d9fdc2796fd1e74804e8bcf7f34940
Summary:
This allows us to sample the most popular method logs (`repo_list_hg_manifest` calls make up for 90% samples in our scuba table) while still have full logging for other queries end errors.
The sampling can be eaily disabled via tunable. In case we get a lot of errors we can also start sampling the error request with a simple configerator change.
Reviewed By: krallin
Differential Revision: D23507333
fbshipit-source-id: c7e34467d99410ec3de08cce2db275a55394effd
Summary:
We want to be able to report more than just on one prefix. Instead, let's add a regex-based reporting. To make deployment easier, let's keep both options for now and later just remove prefix-based one.
Note: this diff also changes how a situation with absent `undesired_path_prefix_to_log` is treated. Previously, if `undesired_path_prefix_to_log` is absent, but `"undesired_path_repo_name_to_log": "fbsource"`, it would report every path. Now it won't report any, which I think is a saner behavior. If we do ever want to report every path, we can just add `.*` as a regex.
Reviewed By: StanislavGlebik
Differential Revision: D23447800
fbshipit-source-id: 059109b44256f5703843625b7ab725a243a13056
Summary:
When bookmarks are moved or created, work out what additional changesets
should have the hooks run on them. This may apply to plain pushes,
force pushrebases, or bookmark only pushrebases.
At first, this will run in logging-only mode where we will count how many
changesets would have hooks run on them (up to a tunable limit). We can
enable running of hooks with a tunable killswitch later on.
Reviewed By: StanislavGlebik
Differential Revision: D23194240
fbshipit-source-id: 8031fdc1634168308c7fe2ad3c22ae4389a04711
Summary:
Move the running of hooks from in `repo_client` to in `bookmarks_movement`.
For pushrebase and plain push we still only run hooks on the new commits the client has sent.
Bookmark-only pushrebases, or moves where some commits were already known, do not run
the hooks on the omitted changesets. That will be addressed next.
The push-redirector currently runs hooks in the large repo. Since hook running has now been moved
to later on, they will automatically be run on the large repo, and instead the push-redirector runs them on
the small repo, to ensure they are run on both.
There's some additional complication with translating hook rejections in the push-redirector. Since a
bookmark-only push can result in hook rejections for commits that are not translated, we fall back to
using the large-repo commit hash in those scenarios.
Reviewed By: StanislavGlebik
Differential Revision: D23077551
fbshipit-source-id: 07f66a96eaca4df08fc534e335e6d9f6b028730d
Summary:
In a repository with files with large histories we run into a lot of SqlTimeout
errors while fetching file history to serve getpack calls. However fetching the
whole file history is not really necessary - client knows how to work with
partial history i.e. if client misses some portion of history then it would
just fetch it on demand.
This diff adds way to add a limit on how many entries were going to be fetched, and if more entries were fetched then we return FilenodeRangeResult::TooBig. The downside of this diff is that we'd have to do more sequential database
queries.
Reviewed By: krallin
Differential Revision: D23025249
fbshipit-source-id: ebed9d6df6f8f40e658bc4b83123c75f78e70d93
Summary:
This is the (almost) final diff to introduce WarmBookmarksCache in repo_client.
A lot of this code is to pass through the config value, but a few things I'd
like to point out:
1) Warm bookmark cache is enabled from config, but it can be killswitched using
a tunable.
2) WarmBookmarksCache in scs derives all derived data, but for repo_client I
decided to derive just hg changeset. The main motivation is to not change the
current behaviour, and to make mononoke server more resilient to failures in
other derived data types.
3) Note that WarmBookmarksCache doesn't obsolete SessionBookmarksCache that was
introduced earlier, but rather it complements it. If WarmBookmarksCache is
enabled, then SessionBookmarksCache reads the bookmarks from it and not from
db.
4) There's one exception in point #3 - if we just did a push then we read
bookmarks from db rather than from bookmarks cache (see
update_publishing_bookmarks_after_push() method). This is done intentionally -
after push is finished we want to return the latest updated bookmarks to the
client (because the client has just moved a bookmark after all!).
I'd argue that the current code is a bit sketchy already - it doesn't read from
master but from replica, which means we could still see outdated bookmarks.
Reviewed By: krallin
Differential Revision: D22820879
fbshipit-source-id: 64a0aa0311edf17ad4cb548993d1d841aa320958
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:
Due to Thrift design of "include" statements in fbcode the thrift structures has to be contained in folders that are identical to the folder layout inside fbcode.
This diff changes the folder layout on Cargp.toml files and in fbcode_builder, there will be a next diff that changes this for ShipIt as well.
Reviewed By: ikostia
Differential Revision: D22208707
fbshipit-source-id: 65f9cafed2f0fcc8398a3887dfa622de9e139f68
Summary:
During expensive getbundle request traversing skiplist uses a lot of cpu, and
in fact it's blocking the whole CPU. krallin suggested to yield since it
should avoid blocking cpus, and that's what this diff is doing.
Reviewed By: krallin
Differential Revision: D22160477
fbshipit-source-id: 5bd881d5c50f4d8e64f2cc90389abc8568ec1df6
Summary: If a value is above cachelib limit let's try to compress it.
Reviewed By: krallin
Differential Revision: D22139644
fbshipit-source-id: 9eb366e8ec94fe66529d27892a988b035989332a
Summary: Megarepo is simplified if we can avoid copying hooks everywhere - run megarepo hooks as well as small repo hooks during pushredirection.
Reviewed By: StanislavGlebik
Differential Revision: D20652331
fbshipit-source-id: f42216797b9061db10b50c1440253de1f56d6b85
Summary:
Remove unused dependencies for Rust targets.
This failed to remove the dependencies in eden/scm/edenscmnative/bindings
because of the extra macro layer.
Manual edits (named_deps) and misc output in P133451794
Reviewed By: dtolnay
Differential Revision: D22083498
fbshipit-source-id: 170bbaf3c6d767e52e86152d0f34bf6daa198283
Summary: Prep for 1.44 but also general cleanups.
Reviewed By: dtolnay
Differential Revision: D22024428
fbshipit-source-id: 8e1d39a1e78289129b38554674d3dbf80681f4c3
Summary:
Let's return FilenodeResult from get_all_filenodes_maybe_stale and change
callers to deal with that.
The change is straightforward with the exception of `file_history.rs`.
get_all_filenodes_maybe_stale() is used here to prefetch a lot filenodes in one
go. This diff changes it to return an empty vec in case filenodes are disabled.
Unfortunately this is not a great solution - since prefetched files are empty
get_file_history_using_prefetched() falls back to fetching filenodes
sequentially from the blobstore. that might be too slow, and the next diffs in
the stack will address this problem.
Reviewed By: krallin
Differential Revision: D21881082
fbshipit-source-id: a86dfd48a92182381ab56994f6b0f4b14651ea14
Summary:
We'd like to serve read traffic even if filenodes are disabled. Let's add a
tunable that can control it.
Reviewed By: HarveyHunt
Differential Revision: D21839672
fbshipit-source-id: 4ec4dd16b9e6e3ffb1ada0d812e1153e1a33a268
Summary: It was replaced with a parameter
Reviewed By: HarveyHunt
Differential Revision: D21839397
fbshipit-source-id: e75900b3da80985cd762659993b8b285411fe928
Summary:
now fastlog will be able to show a history of file like that:
```
Added
|
Deleted
|
Added
```
We do it by using deleted file manifest to find where a file was deleted. We do
it in case we find a unode which doesn't have a parent.
Differential Revision: D21664248
fbshipit-source-id: 8e80fccc87361e66e4e00ae2da3671ed8327f1f8
Summary:
Follow up from D21596758 - current logic of pushrebasing a merge is very
complicated. To prevent repo corruptions let's do a very simple validation -
generate hg changeset from a rebased bonsai changeset.
Given that generating hg changeset is an expensive operation let's do it only
after the first rebase attempt - otherwise we might risk constantly losing the
pushrebase race.
Differential Revision: D21659187
fbshipit-source-id: f43a855cf0fbdbd11a40d3ec38283af344cde5e6
Summary:
Rather than logging `Refreshing tunables` all the time, log only when they
change, and include the values that they are changing from and to.
Reviewed By: farnz
Differential Revision: D21641191
fbshipit-source-id: b0b5edd687e2e41074ce1c8be13fd6918dff924d
Summary:
Currently, there isn't a way to override the value of a tunable during
a unit test. This is unfortunate and may push people to creating integration
tests when unit tests would be best.
Add `with_tunables`, which allows callers to override the tunables values that
a closure accesses. Further, add an example unit test that shows the behaviour.
Thanks to krallin for suggesting this and helping with the implementation
details.
Reviewed By: krallin
Differential Revision: D21595220
fbshipit-source-id: c31da88fb4fcfd38f761960ffe8025132025e6c9
Summary:
Let's add an option to log how many files and trees were fetched in a
particular repo that start with a prefix.
Reviewed By: farnz
Differential Revision: D21617347
fbshipit-source-id: a57f74eadf32781e6c024e18da252c98af21996d
Summary:
Just as for ints and bools, let's add support for strings.
A few notes:
1) we don't have AtomicString, so I decided to use ArcSwap<String>.
However to make code generation simpler (see tunables-derive) I've added a type
alias for that
2) Return type is Arc<String> to avoid unnecessary copies.Another option that we might have here is to put the whole Tunables structure inside ArcSwap, and changing `tunables()` to return Arc<MononookeTunables>
instead of &'static MononokeTunables. However that's a bigger change to make.
Reviewed By: markbt
Differential Revision: D21592390
fbshipit-source-id: 6d3cf340b13f7aef9adb2b1b99ed2bf260033285
Summary:
This adds support for periodically logging that a command is in progress in
Mononoke. The underlying motivation is to make sure that if something is taking
a long time, we can still show some feedback to the user (and log to Scuba). We
might want to log this every 30 seconds.
That said, this is more of an RFC at this stage. I'm thinking it might make
sense to log to Scuba more often and to users less often. It might make sense
to also restrict this to specific commands, such as unbundle:
https://fburl.com/scuba/mononoke_test_perf/atik5959
Reviewed By: StanislavGlebik
Differential Revision: D21549862
fbshipit-source-id: 1d02c5c926abc7e491ac5b8ae0244b5f4620c93e
Summary:
Advertise support for `b2x:infinitepushmutation`. When the client sends us
mutation information, store it in the mutation store.
Reviewed By: mitrandir77
Differential Revision: D20871340
fbshipit-source-id: ab0b3a20f43a7d97b3c51dcc10035bf7115579af
Summary:
Limits on concurrent calls are a bit hard to reason about, and it's not super
obvious what a good limit when all our underlying limits are expressed in QPS
(and when our data sets don't have peak concurrency - instead they have
completion time + # blob accesses).
Considering our past experience with ThrottledBlob has been quite positive
overall, I'd like to just use the same approach in ContextConcurrencyBlobstore.
To be safe, I've also updated this to be driven by tunables, which make it
easier to rollout and rollback.
Note that I removed `Debug` on `CoreContext` as part of this because it wasn't
used anywhere. We can bring back a meaningful implementation of `Debug` there
in the future if we want to. That triggered some warnings about unused fields,
which for now I just silenced.
Reviewed By: farnz
Differential Revision: D21449405
fbshipit-source-id: 5ca843694607888653a75067a4396b36e572f070
Summary:
When we log to Scuba, we need to truncate the `msg` field if it's too long, or
we might be missing log entries in Scuba. I put this behind a tunable so we can, well,
tune it.
Reviewed By: farnz
Differential Revision: D21405959
fbshipit-source-id: 08f0d3491d1a9728b0ca9221436dee2e8f1a17eb
Summary: Let's make it tunable, more context in D21300885
Reviewed By: ikostia, krallin
Differential Revision: D21347636
fbshipit-source-id: 4bc91effdd212a3f5c4a0c3d4952f52a1cf417d7
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
