Summary:
This diff sets two Rust lints to warn in fbcode:
```
[rust]
warn_lints = bare_trait_objects, ellipsis_inclusive_range_patterns
```
and fixes occurrences of those warnings within common/rust, hg, and mononoke.
Both of these lints are set to warn by default starting with rustc 1.37. Enabling them early avoids writing even more new code that needs to be fixed when we pull in 1.37 in six weeks.
Upstream tracking issue: https://github.com/rust-lang/rust/issues/54910
Reviewed By: Imxset21
Differential Revision: D16200291
fbshipit-source-id: aca11a7a944e9fa95f94e226b52f6f053b97ec74
Summary:
This wires up the bookmarks replay logic by connecting the `process_replay_stream` function to an implementation that actually makes bookmark moves through the blobrepo.
This also slightly refactors `process_replay_stream`:
- Use specific errors instead of a generic `failure_ext` `Error`.
- Add logging + Scuba reporting (and report all moves we attempted to make for a given bookmark, as well as their outcome).
- Support passing in a closure instead of a static function, since we need a closure to get a `BlobRepo` reference into the bookmark replay function.
Reviewed By: StanislavGlebik
Differential Revision: D15902312
fbshipit-source-id: d6d57ba880066677748137d684e10796fb8633d5
Summary: See bottom diff for details of the problem we are trying to solve.
Reviewed By: krallin
Differential Revision: D15855497
fbshipit-source-id: a69baaeb98c5257d7ff97206ecf508acda576989
Summary:
See bottom diff for more context on the problem we are trying to solve.
This is a preparation for commit retries.
Reviewed By: krallin
Differential Revision: D15855498
fbshipit-source-id: 6675cc2d698a985bc2ee7d54da00173ba192bc6b
Summary:
See bottom diff for more context on the problem we are trying to solve.
This function can be used to use some more complex decision-making logic than
just `attempt < retry_num` for whether to retry or not. This also does not
have any delay, since in the envisioned use-case delays are not needed.
NB: `#[allow(dead_code)]` will be removed in the later diff. This is just for
the sake of diff separation.
NB 2: I think we should extract all of the different `retry` implementations into a separate crate, but I don't want to do it as part of this task.
Reviewed By: krallin
Differential Revision: D15855496
fbshipit-source-id: a47ccdb507634b202feecfa7f6076decd9451f18
Summary:
This enum represents errors which prompt us for different actions:
- `LogicError` means that retry should happen somewhere in the pushrebase code
- `Other` means infrastructure error
- `RetryabeError` means that the transaction should be retried right away
Next diff will add retries.
Reviewed By: krallin
Differential Revision: D15834461
fbshipit-source-id: 4e2f65b1b1fd8669c29dd22c2920cf88273adb3a
Summary:
This is the first step towards not skipping bundles due to race conditions
in our queue logic.
Below is our hypothesis of why this might happen and how we are trying to fix
it.
```
T | Mononoke Server 1 | Mononoke Server 2 | Sync job
---------------------------------------------------------------------
1 | start transaction | |
---------------------------------------------------------------------
2 | | start transaction |
---------------------------------------------------------------------
3 | mysql decides auto | |
| incremented key 20 | |
---------------------------------------------------------------------
4 | | mysql decides auto |
| | incremented key 21 |
---------------------------------------------------------------------
5 | | INSERT |
---------------------------------------------------------------------
6 | | COMMIT |
---------------------------------------------------------------------
7 | | | read the
| | | latest key (21)
---------------------------------------------------------------------
8 | INSERT | | update the latest
| | | key counter
---------------------------------------------------------------------
9 | COMMIT | |
---------------------------------------------------------------------
```
Note that in the above scenario key 20 will never be read by the sync job,
therefore bundle with this key will never be replayed.
To fix this and also to provide total order of all bookmark moves, we are
will not rely on the `AUTO_INCREMENT` mysql feature, but rather set the new
id ourelves, using the previously computed `MAX(id)` query. Note that it is
fine to insert a predetermined value into an `AUTO_INCREMENT` field.
Clearly, since `SELECT MAX(id)` and `INSERT INTO ...` are two different
queries, the behavior as introduced in this diff is racy, e.g. some other
bookmark move might've already used the `MAX(id)` key we've computed before
our insert succeeds. There are two aspects of this:
- how dangerous it is? In such case transaction commit fails and we fall back
to pushrebase retry logic. We cannot introduce data inconsistency this way.
- what performance hit it introduces? We do not know, maybe it causes many more
retries to happen. To address this potential hit, the following diff will
make sure that when we have reasons to believe that the failure is because of
the mentioned race, we only retry the database transaction, not the entire
pushrebase.
Always winning this race (potentially after a few retries) is exactly what we want
to ensure that we have a total order of all the bookmark moves.
NB: I will not land this diff before introducing the next one
Reviewed By: krallin
Differential Revision: D15821895
fbshipit-source-id: cfb8180d1f43caa9282f06d7dd6a55fd856b71b8
Summary:
CachedBookmarksTransaction drops the cache when a transaction is
committedd, even if the the transaction doesn't modify any bookmarks.
Keep track of whether bookmarks have been modified and only purge the bookmark
cache if this is the case.
Reviewed By: krallin
Differential Revision: D15560228
fbshipit-source-id: f311a17414f2762397084b0b3fdcb7efae551be7
Summary:
This adds a sanity check that limits the count of matches in `list_all_bookmarks_with_prefix`.
If we find more matches than the limit, then an error will be returned (right now, we don't have support for e.g. offsets in this functionality, so the only alternative approach is for the caller to retry with a more specific pattern).
The underlying goal is to ensure that we don't trivially expose Mononoke to accidental denial of service when a list lists `*` and we end up querying literally all bookmarks.
I picked a fairly conservative limit here (500,000), which is > 5 times the number of bookmarks we currently have (we can load what we have right now successfully... but it's pretty slow);
Note that listing pull default bookmarks is not affected by this limit: this limit is only used when our query includes scratch bookmarks.
Reviewed By: StanislavGlebik
Differential Revision: D15413620
fbshipit-source-id: 1030204010d78a53372049ff282470cdc8187820
Summary:
This updates our receive path for B2xInfinitepush to create new scratch bookmarks.
Those scratch bookmarks will:
- Be non-publishing.
- Be non-pull-default.
- Not be replicated to Mercurial (there is no entry in the update log).
I added a sanity check on infinite pushes to validate that bookmarks fall within a given namespace (which is represented as a Regexp in configuration). We'll want to determine whether this is a good mechanism and what the regexp for this should be prior to landing (I'm also considering adding a soft-block mode that would just ignore the push instead of blocking it).
This ensures that someone cannot accidentally perform an infinitepush onto master by tweaking their client-side configuration.
---
Note that, as of this diff, we do not support the B2xInfinitepushBookmarks part (i.e. backup bookmarks). We might do that separately later, but if we do, it won't be through scratch Bookmarks (we have too many backup bookmarks for this to work)
Reviewed By: StanislavGlebik
Differential Revision: D15364677
fbshipit-source-id: 23e67d4c3138716c791bb8050459698f8b721277
Summary:
This adds support for recording server-side whether a given bookmark is publishing and / or pull-default. This is a change on the way towards supporting Infinite Push in Mononoke. This diff will require schema changes on `xdb.mononoke_production`.
There isn't a whole lot of new user-facing functionality in this particular diff. For starters, nobody can edit this flag on bookmarks, and pushes that create a new bookmark will always result in setting a bookmark as publishing AND pull_default.
What this change does however introduce is the notion of `BookmarkHgKind`, which is represents the behavior of this bookmark as far as Mercurial operations are concerned.
There are 3 such kinds, which are relevant in different parts of the codebase:
- PullDefault - this is useful when we want to respond to listkeys queries.
- Publishing — this is useful when we want to identify commit Phases.
- All - this is useful when we want to respond to listkeyspatterns.
Note that only the first two groups are cached in CachedBookmarks.
---
There are a few things going on in this diff (which logically have to happen together):
- First, I updated the `Bookmarks` trait and its various implementations to expose new methods to select Bookmarks based on their hg kind. There's one method per hg kind, and all the methods use a `Freshness` parameter to determine whether to hit cache or not.
- Second, I refactored the various bookmark-access methods in blobrepo. A few of them were duplicative of each other, and some were unused, which became a bigger problem now that we had more (for publishing, pull default, etc.). We are now down to just one method that doesn't hit cache (which is used only by the blobimport script and some tests — perhaps this could be changed?).
- Third, I updated the call sites for the methods that were udpated in step 2 to use the proper method for their use case.
---
Here's a summary of where each method is used (I'm only listing stuff that isn't unit tests):
- `get_bonsai_heads_maybe_stale`:
- `SqlPhases`'s `get_public`
- `build_skiplist_index`
- `get_bonsai_publishing_bookmarks_most_recent`:
- Blobimport (perhaps we can update this to a `maybe_stale` read?)
- `get_pull_default_bookmarks_maybe_stale`
- `listkeys` implementations
- `get_publishing_bookmarks_maybe_stale`
- API Server's `get_branches`
- `get_bookmarks_by_prefix_maybe_stale`:
- `listkeyspatterns` implementation
---
As an aside, note that a lot of the code changes in this diff are actually in CacheBookmark's tests — I had to update those to minimize sleeps: it was fine to sleep in the earlier tests, but I introduced new quickcheck-based tests, and sleeping in those isn't ideal.
Reviewed By: StanislavGlebik
Differential Revision: D15298598
fbshipit-source-id: 4f0fe80ea98dea8af0c8289db4f6f4e4070af622
Summary:
As part of adding support for infinitepush in Mononoke, we'll include additional server-side metadata on Bookmarks (specifically, whether they are publishing and pull-default).
However, we do use the name `Bookmark` right now to just reference a Bookmark name. This patch updates all reference to `Bookmark` to `BookmarkName` in order to free up `Bookmark`.
Reviewed By: StanislavGlebik
Differential Revision: D15364674
fbshipit-source-id: 126142e24e4361c19d1a6e20daa28bc793fb8686
Summary: I'm going to be doing some work on this file, but it's not up-to-date with rustfmt. To minimize merge conflicts and simplify diff reviews, I ran that earlier.
Reviewed By: StanislavGlebik
Differential Revision: D15364676
fbshipit-source-id: 691e00e091e68ce55bc67b29848284a8fedec359
Summary:
First and foremost, this is a safe diff to land on its own as this query
is only used by the sync job and only with the `limit=1`. So the things I am
introducing are not changing any existing behavior.
General goal of this diff is to make sure that these queries always return
series of bookmark update log entries where each entry has the same reason
and bookmark. This way it is always safe to merge these entries into a single
combined entry and send this entry to Mercurial servers for replay.
NB: this same can obviously be done by a nested query, but it is a bit more convenient for me to write it this way. It can be changed if people have strong feelings about it, either now or in a separate diff.
Reviewed By: krallin
Differential Revision: D15251977
fbshipit-source-id: 028c085bb7c4c325c1926bf351b985ef1200ef41
Summary:
At the moment verify integrity script blocks master commits that do not pass
the check. Non-master commits are not blocked even if they don't pass the check
(for example, if they don't have a valid reviewer), but verify_integrity hook
still logs them to scuba.
Mononoke's verify_integrity was enabled only on master, meaning that the hook
won't run on non-master commits at all, and we won't log non-master commits to scuba.
This diff fixes it.
Reviewed By: farnz
Differential Revision: D15146725
fbshipit-source-id: ab432fb2eccae0fbcc10755f5c8447964c490285
Summary:
This introduces prefetching in the hg sync job, which allows us to separate two logically separate steps, and buffer between them:
- Fetching bundle data from Manifold so it can be replayed in Mercurial (we call this "preparing"). This can happen out of order.
- Submitting the changes to Mercurial. This must happen sequentially.
The upshot is that we can fetch new data from Manifold while we wait for Mercurial to apply it, which should increase our throughput.
---
This required a bit of refactoring for the main loop here. The reason for that is that, currently, we heavily rely on closures to capture the `log_entry` so we can pass it to Scuba, etc.
That doesn't work if we want to have buffering, because there isn't a single scope that represents
Instead, every step of the stream has its own scope, so we need to pass the `log_entry` through the stream itself. We also need to track the log entry in errors as well (e.g. to report them to Scuba).
Reviewed By: ikostia
Differential Revision: D15146682
fbshipit-source-id: f83284571f6ca90cb621c621c5165f7fbafd81b5
Summary:
I introduced several new methods on the Bookmarks trait in:
- D15097549
- D15081759
- D15097935
At the same time, Pavel introduced a new implementation of that trait in CachedBookmarks:
- D14928419
That broke the build, this fixes it.
Reviewed By: farnz
Differential Revision: D15124562
fbshipit-source-id: 61e49261c3dc5bb396883e69c29afa329d5df9ab
Summary: This adds the ability to exclude blobimport entries when querying the count of remaining entries in the HG sync replay log.
Reviewed By: ikostia
Differential Revision: D15097549
fbshipit-source-id: ae1a9a31f51a044924fdebbdd219fff1e2b3d46a
Summary:
This introduces a new `--skip` flag in Mononoke admin under `hg-sync-bundle last-processed`. This will update the last-synced counter to the last blobimport that preceeds a log entry that is no a blobimport.
In other words, it makes it so that the last change to be processed is *not* a blobimport.
It will fail if:
- There is no valid log entry to jump ahead to (e.g. all the further log entries are blobimports).
- The current change to be processed is not a blobimport.
- The mutable counter was changed by someone else in the meantime.
Reviewed By: ikostia
Differential Revision: D15081759
fbshipit-source-id: 8465321b08d9c7b5bc97526400518bcf3ac77f13
Summary: This adds a command in mononoke admin to verify the consistency of remaining bundles to sync -- i.e. whether all bundles are blobimports or all of them are not blobimports.
Reviewed By: ikostia
Differential Revision: D15097935
fbshipit-source-id: a0df221c38e84897213edf232972ba420977e9d3
Summary: Now it is possible to configure and enable/disable bookmark cache from configs
Reviewed By: StanislavGlebik
Differential Revision: D14952840
fbshipit-source-id: 3080f7ca4639da00d413a949547705ad480772f7
Summary:
This diff adds caching layer for bookmarks. It only effects semantics of `list_by_prefix_maybe_stale`, other should methods should not be effected by this change.
- all requests to database issued by `list_by_prefix_maybe_stale` are cached for `ttl` time
- but if happened locally cache is purged and replaced to new one which will go through master replica
Reviewed By: StanislavGlebik
Differential Revision: D14928419
fbshipit-source-id: 30994f9c31f8063e8d6b2dd0a6afb06c067aa65a
Summary:
Add a LABEL constant to the SqlConstructors trait to make it easier to identify
which table is being used, for stats and logging.
Reviewed By: HarveyHunt
Differential Revision: D13457488
fbshipit-source-id: a061a9582bc1783604f249d5b7dcede4b1e1d3c5
Summary: We'll do batching to save time on the sync job. We need to sync faster
Reviewed By: ikostia, farnz
Differential Revision: D14929027
fbshipit-source-id: 3139d0ece07f344cdafa5e39b698bc3b02625f0a
Summary:
Add a functionality to show the log of a bookmark i.e. show previous positions of the bookmark. It should look like
mononoke_admin bookmarks log master
2d0e180d7fbec1fd9825cfb246e1fecde31d8c35 push March 18, 15:03
9740f4b7f09a8958c90dc66cbb2c79d1d7da0555 push March 17, 15:03
b05aafb29abb61f59f12fea13a907164e54ff683 manual move March 17, 15:03
...
Reviewed By: StanislavGlebik
Differential Revision: D14639245
fbshipit-source-id: 59d6a559a7ba9f9537735fa2e36fbd0f3f9db77c
Summary: To verify that BookmarkCache is actually needed we want to collect data on how bookmarks are queried
Reviewed By: StanislavGlebik
Differential Revision: D14560748
fbshipit-source-id: ce08511b98c3566cc6ed9052180d73f6076c68fe
Summary: Should be in sync with D14424208
Reviewed By: markbt
Differential Revision: D14541101
fbshipit-source-id: 2e1d544081cd7dd336a76d0490ee91e9137cef55
Summary: To learn how far behind are we in the absolute bundle numbers.
Reviewed By: StanislavGlebik
Differential Revision: D14491672
fbshipit-source-id: 31d16f115b2b6fe4b88c25a847ce229e123b048b
Summary:
We want to be able to reason about the progress of the sync job.
List of fields logged to [`mononoke_hg_sync`](https://our.intern.facebook.com/intern/scuba/query/?dataset=mononoke_hg_sync&pool=uber):
- reason
- entry
- repo_id
- succes
- err
- delay
- bookmark
Reviewed By: StanislavGlebik
Differential Revision: D14243842
fbshipit-source-id: 8ee3591be8f1dc9e1adca081b7b8eb51e7c6521a
Summary:
Before this diff `hg push` that doesn't move a bookmark will fail with `bookmark transaction failed` error.
I don't think it's an expected behaviour. The reason seems to be in the difference of the number of affected rows - mysql returns 0 while sqlite returns 1.
Note that I can't repro the same behaviour in unit-tests, so I assume sqlite
have different semantics from mysql in case of no-op updates.
Reviewed By: ikostia
Differential Revision: D14070737
fbshipit-source-id: e384074dade2b5a7296331ef2fe2da88508c691e
Summary:
First stab at a job that will keep hg in sync with Mononoke when Mononoke
becomes a source of truth.
Reviewed By: ikostia
Differential Revision: D14018269
fbshipit-source-id: f88c5eba8bf5482f2f162b7807ca8e41a3b4291d
Summary:
Previously an error was raised if we tried to commit a transaction that already
existed. Instead let's return `false` which indicates that transaction was
reverted
Reviewed By: ikostia
Differential Revision: D14055392
fbshipit-source-id: a2e78f8c4609a272fe41a1d73478d2ce5503a962
Summary:
Previously it fetched data for all repos. It'd be more useful if we fetch just
for one.
We may later want to replay for many repos at once. When that's the case we can
add a new method.
Reviewed By: ikostia
Differential Revision: D14028467
fbshipit-source-id: e047a891cc920047596ff9221c62ef5cb0090598
Summary:
This diff does not change anything on it's own, but rather adds the not
reachable (but already somewhat tested) code to preserve bundles when doing
pushes and pushrebases.
I want to land it now so that conflict resolution is easier.
Reviewed By: StanislavGlebik
Differential Revision: D14001738
fbshipit-source-id: e3279bc34946400210d8d013910e28f8d519a5f8
Summary:
Together with logging bookmark moves, let's also log bundle handle. It will be
used during replay from Mononoke to mercurial.
Reviewed By: ikostia
Differential Revision: D13990929
fbshipit-source-id: 4039322903b13e84fb31c8e65cc2e097ca765213
Summary:
This is the first step in adding support for tracking all bookmark moves. They
will be recorded in the separate mysql table in the same transaction as
bookmark is updated.
That gives us two things:
1) Ability to inspect all bookmark moves and debug issues with them
2) Also record which mercurial bundle moved a bookmark if any so that we could
later replay these bundles in correct order on hg
Add a struct that let us track bookmark moves.
Reviewed By: ikostia
Differential Revision: D13958872
fbshipit-source-id: 9adfee6d977457db5af4ad5d3a6734c73fcbcd76
Summary:
These are **not** the schemas that we use in production.
So at the moment they are not used for anything and they just create confusion.
Reviewed By: aslpavel
Differential Revision: D13986001
fbshipit-source-id: 7aae0a5da474f579c9cdf1bbf5dfe183835cae2d
Summary: The Copy trait means that something is so cheap to copy that you don't even need to explicitly do `.clone()` on it. As it doesn't make much sense to pass &i64 it also doesn't make much sense to pass &<Something that is Copy>, so I have removed all the occurences of passing one of ouf hashes that are Copy.
Reviewed By: fanzeyi
Differential Revision: D13974622
fbshipit-source-id: 89efc1c1e29269cc2e77dcb124964265c344f519
Summary:
It breaks the pushrebase test.
Original commit: 4e084bee13ff4941d1a42d1f75fe501575858a63
Original diff: D13573105
Reviewed By: StanislavGlebik
Differential Revision: D13651039
fbshipit-source-id: b67c32e0fc4acc953265a089e746ede3d4426b6f
Summary:
After some discussion with Pavel Aslanov, Lukas Piatkowski and Stanislau Hlebik, it was evident that shared future is the best approach for the bookmarks cache.
The cache in this implementation maintains a shared future for each repo, fetching the full list of bookmarks. When a list of bookmarks with a given prefix is required, a filter is applied to a full list future.
Two locks are used in this implementation: one for adding new repos to the hashtable and one for updating the cache. In both cases the optimistic strategy: "let's first first grab a read lock and try checking if it is good enough" is applied.
Reviewed By: StanislavGlebik
Differential Revision: D13573105
fbshipit-source-id: 4e084bee13ff4941d1a42d1f75fe501575858a63
Summary: There's nothing Mercurial-specific about identifying a repo. This also outright removes some dependencies on mercurial-types.
Reviewed By: StanislavGlebik
Differential Revision: D13512616
fbshipit-source-id: 4496a93a8d4e56cd6ca319dfd8effc71e694ff3e
Summary:
Removed:
cmd-line cmd tool for filenodes and bookmarks. These should be a part of
mononoke_admin script
Outdates docs folder
Commitsim crate, because it's replaced by real pushrebase
unused hooks_old crate
storage crate which wasn't used
Reviewed By: aslpavel
Differential Revision: D13301035
fbshipit-source-id: 3ae398752218915dc4eb85c11be84e48168677cc
Summary:
Currently we read all bookmarks from primary replica a few times during `hg
pull`. First time when we do listkeys, second time when we get heads.
That might create a high load on primary replica.
However the delay between primary and secondary replicas is fairly small, and so it
*should* be fine to read bookmarks from secondary local replica as long as there is only
one replica per region (because if we have a few replicas per region, then
heads and listkeys response might be inconsistent).
Reviewed By: lukaspiatkowski
Differential Revision: D13039779
fbshipit-source-id: e1b8050f63a3a05dc6cf837e17a448c3b346b723
