Summary:
This diff prepares the Mononoke codebase for composition-based extendability of
`ScubaSampleBuilder`. Specifically, in the near future I will add:
- new methods for verbose scuba logging
- new data field (`ObservabilityContext`) to check if verbose logging should
be enabled or disabled
The higher-level goal here is to be able to enable/disable verbose Scuba
logging (either overall or for certain slices of logs, like for a certain
session id) in real time, without restarting Mononoke. To do so, I plan to
expose the aforementioned verbose logging methods, which will run a check
against the stored `ObservabilityContext` and make a decision of whether the
logging is enabled or not. `ObservabilityContext` will of course hide
implementation details from the renamed `ScubaSampleBuilderExt`, and just provide a yes/no
answer based on the current config and sample fields.
At the moment this should be a completely harmless change.
Reviewed By: krallin
Differential Revision: D25211089
fbshipit-source-id: ea03dda82fadb7fc91a2433e12e220582ede5fb8
Summary:
cross repo bookmark validation alarm fired a few times, and looks like it fired
because of the following:
1) find_bookmark_diff compared boomarks and found an inconsistency for bookmark
BM which points to commit A in large repo. Next step is to check bookmark history
2) While find_bookmark_diff was running a new commit B was landed in a large repo
and was backsynced to the small repo, so BM now points to commit B.
3) check_large_bookmark_history is called and it fetches latest bookmark log entries, and
it gets entries for commit A and commit B. check_large_bookmark_history checks
if any of the fetched entries points to a commit in the small repo and if yes then
it also checks if this bookmark update happened not so long ago. And the
problem is in the way it checks the "not so long ago" part. It does so by
finding the time difference between latest bookmark update log entry and any
other bookmark update log entry.
Now, if time difference between these two log entries (for commit B and for
commit A) is more than max_delay_secs (which happens only
if commit rate is low e.g. during the weekends), then the alarm would fire
because the delay between latest bookmark update log entry (the one that moved
BM to commit B) and previous log entry (the one that moved BM to commit A) is too large.
This diff fixes this race by skipping newest entries until we found a bookmark
update log entry which points to the large commit that find_bookmark_diff
returned.
Reviewed By: ikostia
Differential Revision: D25196760
fbshipit-source-id: dfa0dca0001b1c38759ec9f4f790cfa3197ae2cf
Summary:
- convert save_bonsai_changesets to new type futures
- `blobrepo:blobrepo` is free from old futures deps
Reviewed By: StanislavGlebik
Differential Revision: D25197060
fbshipit-source-id: 910bd3f9674094b56e1133d7799cefea56c84123
Summary: convert `BlobRepo::get_bonsai_bookmark` to new type futures
Reviewed By: StanislavGlebik
Differential Revision: D25188577
fbshipit-source-id: fb6f2b592b9e9f76736bc1af5fa5a08d12744b5f
Summary: Let's use fbinit::compat_test, it makes the tests a bit shorter
Reviewed By: ikostia
Differential Revision: D25196759
fbshipit-source-id: bd8aca4b676a71158269195fd35153a0934b0cbc
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:
We got a few ubns because one bookmark validator in a single region wasn't able
to connect to mysql and was reporting errors.
This diff fixes by separating logical and infra errors
Reviewed By: ikostia
Differential Revision: D25092364
fbshipit-source-id: 93f4be1a7e0467051b7b8d927eef9b4f5cd6a983
Summary: Now that `derive03` is the only version available, rename it to `derive`.
Reviewed By: krallin
Differential Revision: D24900106
fbshipit-source-id: c7fbf9a00baca7d52da64f2b5c17e3fe1ddc179e
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: As part of the effort to deprecate futures 0.1 in favor of 0.3 I want to create a new futures_ext crate that will contain some of the extensions that are applicable from the futures_01_ext. But first I need to reclame this crate name by renaming the old futures_ext crate. This will also make it easier to track which parts of codebase still use the old futures.
Reviewed By: farnz
Differential Revision: D24725776
fbshipit-source-id: 3574d2a0790f8212f6fad4106655cd41836ff74d
Summary:
This seems to trip up Cargo builds
```
error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `with`
--> src/lib.rs:365:3
|
7 | S with version V1
| ^^^^ expected one of 8 possible tokens
error: aborting due to previous error
```
Reviewed By: StanislavGlebik
Differential Revision: D24754708
fbshipit-source-id: 0dc5539acf340ac409bf7b6158313c8fec16a275
Summary:
This diff changes semantic of `sync_commit()` function to return an error when
trying to sync a commit with no parents. This is a small code change which has big change
in semantics, and because of that I had to change how backsyncer and
mononoke_x_repo_sync job.
Instead of using `unsafe_sync_commit()/sync_commit()` functions both backsyncer and
`x_repo_sync_job` now use `unsafe_sync_commit_with_expected_version()`
which forces them to specify which version to use for commit with no parents.
And in order to find this version I changed find_toposorted_unsynced_ancestors
to not only return unsynced ancestors but also return mapping versions of their
(i.e. of unsynced ancestors) parents. Given this mapping we can figure out what
version is supposed to be used in `unsafe_sync_commit_with_expected_version()`.
The question raises what to do when a commit doesn't have any synced ancestor and hence we can't decide
which version to use to remap it. At the moment we are using current version (i.e. preserving the existing behaviour).
However this behaviour is incorrect, and so it will be changed in the next diffs
Reviewed By: ikostia
Differential Revision: D24617936
fbshipit-source-id: 6de26c50e4dde0d054ed2cba3508e6e8568f9222
Summary:
Previously we were always choosing the current version for remapping via
pushrebase, but this is incorrect. Let's instead select the version based on
what version parent commits used to remap with.
Reviewed By: ikostia
Differential Revision: D24621128
fbshipit-source-id: 2fedc34b706f090266cd43eaf3439f8fb0360d0d
Summary: This is to be able to see in Scuba why a given sync function was called.
Reviewed By: StanislavGlebik
Differential Revision: D24689366
fbshipit-source-id: f868fc1b6fcbf6c692e1373cbe8da8cd4a230780
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:
This didn't matter much in practice because we've never pushrebased a rewritten
commit where one of the parents rewrites to nothing. Nevertheless I think it's
cleaner to return an error in this case
Reviewed By: ikostia
Differential Revision: D24621105
fbshipit-source-id: 8cf62efd28e0c9aed945f727b1872db6922cfeb3
Summary:
This stack has a goal of cleaning up how commits with no parents and
consequently merges are treated in CommitSyncer. At the moment it always uses current version
to rewrite them, and this is incorrect. See example below (here N means "new
commits" i.e. commits that weren't remapped yet but we need to remap them now)
```
large repo
O <- uses current version
|
O
|
O N
|/ |
O | <- uses old version
| N
| |
O N <- this commit should be rewritten with old version!
```
With current logic all N commits will use current version for remapping, but
this is incorrect, and instead "old version" should be used. The goal is to fix
it and make it so that backsyncer and x-repo sync job pick the correct version
to use for remapping.
-----
As it was noted in the previous diff we need a function that overrides a
version for a commit with no parents. We need this function because for a
commit with no parents CommitSyncer can't decide which mapping version to use
because, well, there are no parents which could have hinted about the version.
So let's add this function and cleanup unsafe_sync_commit_impl function to
apply version override only to a commit with no parents. If commit has parents then we'd verify that the version from parents matches the expected version.
In the next diffs I'll make it so that if a version for commit with no parents is not specified then
unsafe_sync_commit_impl fails rather than uses current version.
Reviewed By: ikostia
Differential Revision: D24617953
fbshipit-source-id: a68f837da9d90bb18034f628b7880482a2e548b7
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: This adds a `ScubaSampleBuilder` field to the `CommitSyncer` and ensures this field is instantiated with correct values (real vs discarding sample) depending on circumstances.
Reviewed By: StanislavGlebik
Differential Revision: D24539732
fbshipit-source-id: 37aedcff9aefcfcd6b740a0491ab35f9e5ce7c77
Summary:
Functionality of this function can be replaced with
unsafe_always_rewrite_sync_commit, so let's do that.
However in the next diff we'll need a function similar to
test_unsafe_sync_commit but with a slightly different semantics - instead of
forcing override for every commit we'll just force it for a commit with no
parents. Because of that let's not remove unsafe_sync_commit_impl function.
Reviewed By: ikostia
Differential Revision: D24616824
fbshipit-source-id: 6969145d98cd23604920a6b490bf7ffe47938c08
Summary:
`CommitSyncerArgs` was useful when `CommitSyncer` did not have a way to query
existing configs from the configerator. Now that `SyncDataProvider` is part of
`CommitSyncer`, we can get rid of `CommitSyncerArgs`, which will also make
further improvements more convenient.
Reviewed By: StanislavGlebik
Differential Revision: D24565773
fbshipit-source-id: 4dd507b06d946c6018a4a4e8d5e77c6b27abe195
Summary: A better fix would be to also get rid of `Option` in the struct itself, but I don't want to spend any more time on this atm, and this change is a clear improvement.
Reviewed By: StanislavGlebik
Differential Revision: D24538309
fbshipit-source-id: 6190c6936dc34d996ecd3d700e5f71282d45f651
Summary:
update Phases::add_reachable_as_public to futures03
With this change all the Phases methods are futures03
Reviewed By: krallin
Differential Revision: D24531552
fbshipit-source-id: e9201621b798739d4d7dd197f15188103e9d359a
Summary:
Fewer clones, better code.
Note that in some cases we would previously have a fn that takes `ctx` by
ownership and just passes it through to some other fn outside of the
`cross_repo_sync`. I triead to make all such functions borrow and clone instead
in order to push cloning to the leaf fns of `cross_repo_sync`.
Reviewed By: StanislavGlebik
Differential Revision: D24538028
fbshipit-source-id: 8a3e78d4076b34d8b1767cdee1db3fdbb7acb4f7
Summary: In D24447404, I provided some utility functions that allowed me to avoid constructing and/or passing around a ConfigStore. Remove those functions and fix up the code to run.
Reviewed By: krallin
Differential Revision: D24502692
fbshipit-source-id: 742dbc54fbcf735895d6829745b9317af14dfa0b
Summary: It's designed as a singleton store for normal use - rather than have lots of ways to get different config stores, let's use one global store
Reviewed By: krallin
Differential Revision: D24447404
fbshipit-source-id: 6ecc46351b14794471f654cec98527a11a93d3ef
Summary:
- convert ManifestOps to new style futures
- at this point `//eden/manifest:manifest` crate is completely free from old style futures
Reviewed By: krallin
Differential Revision: D24502214
fbshipit-source-id: f1cdb11bd8234f22af5c905243f71e1e9fca11f1
Summary: Somewhat more convenient to work with, no need to look inside.
Reviewed By: StanislavGlebik
Differential Revision: D24474898
fbshipit-source-id: 7ee0920e7d0d5a2102c68695a5cc0d9e237d958d
Summary:
I removed --debug flag for bookmarks validator on tw because it was too spammy,
but now we get no output at all. Let's make it info so that we can see it in tw
output
Reviewed By: ikostia
Differential Revision: D24445829
fbshipit-source-id: d37c41be46794867f91ecf66f0318e7b2d660d85
Summary: As we saw today it was showing entries in incorrect order
Reviewed By: ikostia
Differential Revision: D24418800
fbshipit-source-id: c926c7904338ae40b26fe08cb1ac973e384bad28
Summary:
Prior to this diff, validation logic did not work when target repo was a large
one (although this limitation wasn't advertised). This was becase we explicitly
never apply a `Mover` to the target repo paths and therefore expect every
single target repo path to have an equivalent in the source repo. This is true
when the target repo is small, but not true when it is large.
It is pretty easy to make this logic direction-agnostic: just proactively check
whether target repo paths rewrite into nothingness or not.
Reviewed By: StanislavGlebik
Differential Revision: D24399029
fbshipit-source-id: 9ca5bb03760e662ff6756c27c0c77204abdb38de
Summary: This command will help us understand if it is safe to enable push-redirection with a certain CommitSyncConfigVersion,
Reviewed By: StanislavGlebik
Differential Revision: D24345898
fbshipit-source-id: c2a4c034a9203025ce3534986b3bb986784ff2b1
Summary:
Previously it was just taking the current version. Instead of guessing let's
allow users to specify whatever version they want to rewrite it with.
Reviewed By: ikostia
Differential Revision: D24360918
fbshipit-source-id: d9f4c55cd66931a9f2ab7da3474d2152d77525d0
Summary:
update_mapping method was silently setting current mapping version. This was
very error-prone and we had a replacement update_mapping_with_version. This
diff removes update_mapping completely.
Reviewed By: krallin
Differential Revision: D24360105
fbshipit-source-id: 57761f4279f75032e9d4ec88a45e5199e250247a