Summary:
This adds a split_err helper, which splits an error from a stream into
a separate future. This allows the stream to be infallable.
Reviewed By: sunshowers
Differential Revision: D15746093
fbshipit-source-id: f2d3c10620365daff497c350865a928bd45da8cf
Summary:
stream_clone() takes a stream of cloneable items (and errors) and clones it n
ways to n streams. There's no buffering - all output streams must consume each
item before the next input is consumed. Output streams can be dropped
independently; the input is dropped if all outputs are dropped.
Reviewed By: Imxset21
Differential Revision: D15746068
fbshipit-source-id: 7cf1e92b36449ae2112c91ef393d885e9e16c0ae
Summary:
When a commit is pushrebased, we generate a new Bonsai commit to represent the result of the commit, which is the commit we actually make public.
Therefore, the commit that will be rebased should be uploaded as a draft. The rebase result will then provide filenodes, and thus we will generate good linknodes for pushrebaseed commits.
Reviewed By: krallin
Differential Revision: D16550469
fbshipit-source-id: 1f2f00f8cd9ad0f75441aca0eb8daae62ae299e0
Summary:
Mononoke's CoreContext stores PerfCounters that can help with debugging.
The API Server used to parse them only for is_ancestor() thrift requests.
Move the parsing logic into the repo actor, so that the PerfCounters are always
read.
Reviewed By: fanzeyi
Differential Revision: D16540580
fbshipit-source-id: 9c60942c052ee0cdc06f0dcdb857b2fbe008ccc8
Summary:
It's currently challenging to compare client logging to server logging, because we just don't have enough information to correlate logs.
Add in two pieces of data from the client, if available:
1. Client correlator, which lets us compare to client performance logging.
2. Short hg command (`pull`, `update`, `rebase`, but not `update --clean master` or `rebase --source foo --destination master`). This lets us quickly look for problematic commands with a suitable query
Reviewed By: StanislavGlebik
Differential Revision: D16499809
fbshipit-source-id: c17a348c6f950bd207df5296531e5d39124e3a79
Summary: Will be used in the canaries
Reviewed By: HarveyHunt
Differential Revision: D16515265
fbshipit-source-id: 7ed21dce971531899bc8983ec04841aa6d707e09
Summary:
Implement BonsaiDerived trait for root manifest unode. Note that it mainly just
calls `derive_unode_manifest()`
This diff links together code that derived unode manifest for a single commit and derived data library. After that diff is landed we should be able to run `RootManifestUnodeId::derive(...)` and get unodes for any bonsai commit
Reviewed By: krallin
Differential Revision: D16446044
fbshipit-source-id: 37d5a4ab4332fd6018a0bdd2c1be231dca164afc
Summary:
It's not cool to include the `flat/` prefix into the queue. Not cool at all.
It's not part of the actual blobstore key.
Let's make the populator cool again.
Reviewed By: ahornby
Differential Revision: D16494119
fbshipit-source-id: 8d3a34271d19ab0531cbb59e56b7a1ac04f66b07
Summary:
In a very mergy repos we can hit a combinatoric explosion by visiting the same
node over and over again. Derived data framework has the same problem, and this diff
fixes it.
I had a few attempts at implementing it:
**1** Use `bounded_traversal`, but change unfold to filter out parents that were already visited.
That wasn't correct because during fold will be called only with
the "unvisited" parents. For example in a case like
```
D
/ \
C B
\ /
A
```
fold for C or B will be called with empty parents, and that's incorrect.
**2** Use `bounded_traversal`, change unfold to filter out visited parents but
also remember real parents.
That won't work as well. The reason is that fold might be called before unfold
for parents have finished. so in the case like
```
D
/ \
C B
\ /
A
|
...
thousands of commits
```
If C reaches A first, then B won't visit any other node, and it will try to
derive data for B. However derived data for A might not be ready yet, so
deriving data for B might fail.
**3** Change bounded_traversal to support DAGs not just tree.
From two points above it's clear that bounded_traversal should be called
bounded_tree_traversal, because on any other DAGs it might hit combinatoric
explosion. I looked into changing bounded_traversal to support DAGs, and that
was possible but that was not easy. Specifically we need to make sure that all
unfold operations are called after fold operations, stop using integers for
nodes etc. It might also have a perf hit for the tree case, but not clear how
big is it.
While I think supporting DAGs in bounded_traversal makes sense, I don't want to
block derived data implementation on that. I'll create a separate task for that
---------------------------------------------------------------------------------
The approach I took in the end was to use bounded_stream_traversal that don't
visit the same node twice. Doing this will find all commits that need to be
regenerated but it might return them in an arbitrary order. After that we need
to topo_sort the commits (note that I introduced the bug for hg changeset
generation in D16132403, so this diff fixes it as well).
This is not the most optimal implementation because it will generate the nodes
sequentially even if they can be generated in parallel (e.g. if the nodes are
in different branches). I don't think it's a huge concern so I think it worth
waiting for bounded_dag_traversal implementation (see point 3) above)
---------------------------------------------------------------------------------
Finally there were concerns about memory usage from additional hashset that
keeps visited nodes. I think these concerns are unfounded for a few reasons:
1) We have to keep the nodes we visited *anyway* because we need to generated
derived data from parents to children. In fact, bounded_traversal keeps them in
the map as well.
That's true that bounded traversal can do it a bit more efficiently in cases
we have two different branches that do not intersect. I'd argue that's a rare
case and happens only on repo merges which have two independent equally sized
branches. But even for the case it's not a huge problem (see below).
2) Hashset just keep commit ids which are 32 bytes long. So even if we have 1M
commits to generate that would take 32Mb + hashset overhead. And the cases like
that should never happen in the first place - we do not expect to generate
derived data for 1M of commits except for the initial huge repo imports (and
for those cases we can afford 30Mb memory hit). If we in the state where we
need to generate too many commits we should just return an error to the user,
and we'll add it in the later diffs.
Reviewed By: krallin
Differential Revision: D16438342
fbshipit-source-id: 4d82ea6111ac882dd5856319a16dda8392dfae81
Summary:
Running Mononoke locally make it look like that 'Loading hooks' takes a lot of
time, while in reality it's skiplists. let's add logs to make it clearer
Reviewed By: farnz
Differential Revision: D16460545
fbshipit-source-id: 8fe43d2b4efb8b2cdc13a1363f36350fb11cda0a
Summary: `test-censoring-enabled` and `test-censoring-disabled` test if the config option to disable/enable censorship verification works . The diff merges these 2 tests since they test the same thing and the configuration is similar for both of the cases
Reviewed By: krallin
Differential Revision: D16441380
fbshipit-source-id: 97b3cfc6969be32d661d4676d6cdc7cfcd4eed04
Summary:
We have fixed parent order for bonsai changeset i.g. (p1, p2) != (p2, p1). We
need to keep the same in unodes. This diff adds a test for that.
Reviewed By: krallin
Differential Revision: D16446231
fbshipit-source-id: 2e750909496f1b5f84e256e129d61677956b846b
Summary:
This diff implements basic derived data library.
Different derived data types (e.g. hg changesets, unodes, git changesets) need
to implement `BonsaiDerived::derive_from_parents()`, and then it can be used
like
```
RootManifestUnodeId::derive(ctx, repo, mapping, bcs_id)
```
The library takes care of generating changesets for all the parents and saving
generated id into the mapping. For more info see code comments - and do let me
know if anything is unclear.
Reviewed By: krallin
Differential Revision: D15479931
fbshipit-source-id: 8730083de055ce698d3f168ae0265d85123bdaec
Summary:
Any get request for a blacklisted file returns a magic string - not an error anymore - which replaces the actual content of the file. The magic string represents a sequence of 256 random characters.
Put still returns an error if there is an attempt to change a blacklisted file, but that's the normal behavior for this type of request.
Reviewed By: ikostia
Differential Revision: D16333820
fbshipit-source-id: 54e0189ebe2a71ea365979950d44dbb3fd1e5dc7
Summary: To aid with debugging, return the session UUID to the HTTP client in a header.
Reviewed By: krallin
Differential Revision: D16439125
fbshipit-source-id: fa03b9e5aa4940f0a7023859a9a3d3f38b29565b
Summary:
The API server logs different information for Thrift and HTTP requests, such as logging
future poll time for Thrift requests, but not HTTP. In order to add this logging, it would need to
be duplicated for both HTTP and Thrift logic.
Refactor the logging to perform future stats and parameter collection in the repo actor, so that it
is performed in one common place. Further, reduce the duplicated code in the thrift request handling.
Also, update the logging to log two entries to scuba - one when the core repo actor completes its functionality
and another when the HTTP / Thrift handler completes. This allows for some specific interface information (Such
as HTTP return code or Thrift response size) to be logged.
Reviewed By: fanzeyi
Differential Revision: D16417996
fbshipit-source-id: 61ace1eb3253e7293c126805d4cf5dc100491503
Summary:
This seems to be the standard way (e.g. `ManifoldBlob` does it :P) to report
fb303 counters for blobstores. Provided that we want XDB Blobstore to be a
full-featured one, we want this too.
Note: potentially it makes sense to just wrap every blobstore in the `CountedBlob` in `blobstore/factory`, I can work on that later. For now I want to be able to proceed with xdb blobstore and get its counters in the cheapest possible way.
Reviewed By: krallin
Differential Revision: D16441222
fbshipit-source-id: 04fffb4598a7929301f05faacd074d5c0c76848d
Summary:
Unode parents can be ancestors of each other in some rare cases (see comment). We decided
that preventing this case is not worth it - there's a potential perf hit with
no big benefits.
This diff adds a comment that makes this decision explicit.
Reviewed By: krallin
Differential Revision: D16437748
fbshipit-source-id: 95c74954f8dfde637a2ede35888ea24d54cb7415
Summary:
This diff handles a tricky case of merging two files with the same content but
different histories. See code comment for more details.
Reviewed By: krallin
Differential Revision: D16360117
fbshipit-source-id: 9a21473d49b63b7914e72357da63adcc35f5b612
Summary:
First stab at derived unodes. For now it should work, but only for linear
history e.g. no merges. Support for merges is coming in the next diffs.
The logic of deriving unodes is similar to the logic of deriving hg manifests
which was implemented in D16131665
Reviewed By: krallin
Differential Revision: D16333369
fbshipit-source-id: 52e1841295f1faf0306fb2dbce65674bb60b3d93
Summary: Makes it easier to use apiserver scuba table
Reviewed By: HarveyHunt
Differential Revision: D16407281
fbshipit-source-id: fe114fa637e5470833fd5bfec8c58609d3d0e272
Summary:
Before this change, we would always include the shard id in our mysql-related fb303 counters. This is not perfect for two reasons:
- the the xdb blobstore we have 4K shards and 24 counters, so we were reporting 96K counters in total
- we rarely care about per-counter metrics anyway, since in most cases all queries are uniformly distributed
Therefore, let's change this approach to not use per-shard counters and use per-shardmap ones (when sharding is involved).
Reviewed By: krallin
Differential Revision: D16360591
fbshipit-source-id: b2df94a3ca9cacbf5c1f328b48e87b48cd18287e
Summary:
Add a new combinator that can be used to inspect the result of a
future. This is useful when you want to run the same code for inspect_err and inspect.
Also, update the inspect_err combinator to not panic if polled twice - instead
make it act like a fused future.
Reviewed By: farnz
Differential Revision: D16359181
fbshipit-source-id: 14948b851867d5792c76cc679297b23b1e8a6adc
Summary:
Improve memory usage on mononoke startup and reduce number of small allocations. Done via:
* Pre-size CHashMap used by SkiplistEdgeMapping, working around the 4x multiplier and lack of load factor awareness in CHashMap::with_capacity
* Add a SingleEdge optimization to SkiplistNodeType so as to save vector overhead in the case of one edge ( this is the common case )
* Size the HashMap in rust thrift deserialization with HashMap::with_capacity
Reviewed By: krallin
Differential Revision: D16265993
fbshipit-source-id: 99c3a7149493d824a3c00540bc5557410d0273fc
Summary:
Originally that was part of D16361366 which turned BoxStream into struct with
must_use. However we decided to take another approach with must_use
That said, the cleanup is still useful even without must_use
Reviewed By: farnz
Differential Revision: D16376122
fbshipit-source-id: 295e9e71d7da82651e12139d35de5bd300e44153
Summary: As above, add the output for load_limit_config to debug formatting.
Reviewed By: StanislavGlebik
Differential Revision: D16357353
fbshipit-source-id: d5ce5aa3801563de33902c7e2216dcee904db76e
Summary:
The stats prefix for bonsai hg mapping is "bonsai-hg-mapping" in lib.rs and
"bonsai_hg_mapping" in caching.rs. Convert lib.rs's prefix to match that of caching.rs,
making it easier to find stats.
Reviewed By: StanislavGlebik, farnz
Differential Revision: D16357147
fbshipit-source-id: f9c5bfa5a43c9b8c06ac8cb3b250cdf8fe689a28
Summary:
Adds method `collect` that allows to convert a `Stream` into `Future` of
all of its elements where the elements themselves are stored in the generic
collection type of which can be determined by the caller.
Reviewed By: StanislavGlebik
Differential Revision: D16283392
fbshipit-source-id: 27ef22fbf35c1d9bfe6590d50321e62685604e9e
Summary:
My previous diff (D16327788), despite claiming a 1000x improvement, was merely a ~220x
improvement. This diff is some tweaking with numbers, whihc brings about
another 1.3x improvement, bringing the total one to be 290x.
Reviewed By: krallin
Differential Revision: D16356932
fbshipit-source-id: 3d3f0c844eec9866217cf5a57f285fe8a56152de
Summary:
Previously, we were passing a specially created core context that doesn't
use a Scuba logger. Update this to use a CoreContext with a scuba logger in it.
NOTE: The scuba logger in the CoreContext isn't mutable, which means that to add new values to it,
the ScubaSampleBuilder needs to be cloned. However, this means that the contents of the sample will
diverge:
let mut scuba = ctx.scuba().clone();
scuba.add("foo", 1);
// Won't contain foo
ctx.scuba().log();
This is annoying for the API Server as it would be nice to pass CoreContext into the Repo actor and then
log some things from there (Such as params).
Mononoke works around this by cloning the ScubaSampleBuilder after common values have been added
(such as server information) and then logging to scuba at the end of each request. Perhaps we should do this
for the API Server too. i.e. we clone the ScubaSampleBuilder from CoreContext once we enter the Repo actor
code so that we have a central place to record params etc and log once the actor is done, logging another sample
once the Thrift or HTTP code is done too.
Reviewed By: StanislavGlebik
Differential Revision: D16341284
fbshipit-source-id: c28083b04643421d24270e54557552551b938394
Summary:
The response_time middleware was used by both the scuba and slogger middleware.
Now that they're both gone, let's move the response_time into core_context.
Reviewed By: krallin
Differential Revision: D16356804
fbshipit-source-id: e0a5caff8c4fae97d73a7fa6fecd92aa0538ad5f
Summary:
The web part of the API Server has been using fake CoreContexts. Implement a middleware
that creates a new CoreContext for each request and allows the web handles to access it. Further,
move the logic from the ScubaMiddleware and SloggerMiddleware into the CoreContext middleware and remove the code.
Reviewed By: fanzeyi
Differential Revision: D16341282
fbshipit-source-id: dbd9ad2f3ec07314b180ca905df977effdc48130
Summary: Adding throttling to gettreepack requests. Note that it also fixes ods tree counting as it was wrong previously - it was counting blocks of bytes, which made no sense at all. One block of bytes could contain a single tree, multiple trees or even part of a tree.
Reviewed By: krallin
Differential Revision: D16328588
fbshipit-source-id: e5f1869e08cac19f5a662b9a8e3a87e002a5f25c
Summary: The size of entries isn't related to our local pointer size. This updates that.
Reviewed By: StanislavGlebik
Differential Revision: D16336407
fbshipit-source-id: 2f500c54d218d81f06ba0827bd72ad8be1192b9d
Summary: This adds a few retries in create_raw_xdb_connection. This is intended as a first step towards solving some of the flakiness we've observed when connecting to MySQL through direct connections (sometimes, we fail to acquire certificates).
Reviewed By: farnz
Differential Revision: D16228401
fbshipit-source-id: 0804797aecfe0b917099191cd2a36ce4c077b949
Summary:
In earlier diffs in this stack, I updated the callsites that reference XDB tiers to use concrete &str types (which is what they were receiving until now ... but it wasn't spelled out as-is).
In this diff, I'm updating them to use owned `String` instead, which lets us hoist up `to_string()` and `clone()` calls in the stack, rather than pass down reference only to copy them later on.
This allows us to skip some unnecessary copies. Tt turns out we were doing quite a few "turn this String into a reference, pass it down the stack, then turn it back into a String".
Reviewed By: farnz
Differential Revision: D16260372
fbshipit-source-id: faec402a575833f6555130cccdc04e79ddb8cfef
Summary:
This implements and uses the `add_many` method of the blob healer queue. This method allows us to do batched adds, which in turn allows us to use `chunks` on Manifold iteration.
NB 1: I deliberately removed control symbols form progress print message. If we only print it on the same line, we loose it when the job crashes.
NB 2: I deliberately use range of `entries[0]`, as I want to pessimistically restart from the earliest in case of a failure.
Reviewed By: krallin
Differential Revision: D16327788
fbshipit-source-id: 8d9f3cf85ee7cbca657a8003a787b5ea84a1b9b0
Summary:
It's used only in a very few places, and most likely that's by accident. We
pass logger via CoreContext now
Reviewed By: krallin
Differential Revision: D16336953
fbshipit-source-id: 36ea4678b3c3df448591c606628b93ff834fae45
Summary:
We're currently not emitting any copy metadata in LFS in response to getfiles. As it happens, this appears to fine (clients still get the right metadata), because getreepack includes file copy data and so the client isn't bothered by the fact that the LFS pointers we return don't include it.
That said, it's probably a good idea to clean this up and include copy data. After all, we have it, and we should return to the client whatever they sent us.
Reviewed By: farnz
Differential Revision: D16282793
fbshipit-source-id: 6671f87de2acae2c5da1c754510bbb61de5768d3
Summary: We are fetching envelopes twice in repo_commit, this updates the code to provide a shared abstraction over file and manifest envelopes in order to avoid this duplicate fetch.
Reviewed By: farnz
Differential Revision: D16282794
fbshipit-source-id: 53d53f28f051cbacbc1c931b2dbb98ac74a19c15
Summary:
In repo_commit, we already leverage a file or manifest's envelope to gets its parents, but were getting the copy info from the raw filenode bytes (through a call to `get_raw_content`, which for a filenode eventually calls into `fetch_raw_filenode_bytes` to get the content from the blobstore and retrieves the metadata from the envelope, and then proceeds to discard the contents and only retain the copy info).
This updates this code to just get the metadata from the envelope in order to retrieve copy info, which is functionally equivalent, but avoids a fetch to the blobstore.
There's still some potential for improvement here, since we're duplicating the calls to get the envelope for a file. I'm fixing this up in a separate diff for clarity.
Reviewed By: farnz
Differential Revision: D16282795
fbshipit-source-id: eb9068dcc0d36c4a9c0046e856db3fb2e9a3748f
Summary:
This diff is loosely based of D15851686.
There were a few problems with the previous load shedding api. The biggest
problem was that it used static variables that required explicit
initialization. So if someone forgets to add an init in any tool (say, in
mononoke_admin), and then calls the code that runs `should_throttle` then the
binary would just crash.
The second big problem was that on each `bump_load` call configerator config
was parsing json config over and over again. I didn't measure how expensive it
was, but it looked very counter-intuitive that we put load counters in static
variables (IIRC to avoid allocations), but then we reparse the config on each
call.
To avoid these problems this diff moves ratelimiting logic into CoreContext.
The config is parsed only once on CoreContext creation and all global variables
are now local. Because of these changes `loadlimiter` library is no longer
necessary.
Note that with load limiter counters are allocated on each
`bumpLoad/shouldThrottle` call. I'll investigate how expensive it actually is.
One drawback is that now it's trickier to track egressbytes for stderr, and it was removed.
We can add it later if needed
Reviewed By: farnz
Differential Revision: D16221607
fbshipit-source-id: f934cb2fc778df1679acc7d6eb2a1a8b4deb0aac
