* refactor: pull out type
* feat: impl `Loader` for `Arc`
* refactor: have a single `TestLoader`
* refactor: simplify code
* refactor: encapsulate `CancellationSafeFutureReceiver`
Avoid that the querier accesses files that were flagged for deletion a
long time ago. This would happen if the following conditions hold:
- we have very long-running querier pods (e.g. over holidays)
- the table doesn't receive any writes (or the partition if we ever
change the cache granularity), hence the querier is never informed
that its state is out-of-date
- a compactor runs a cold compaction, and by doing so flags a file for
deletion
- the GC finally wants to delete it
This is mostly a safety measure to prevent weird internal server errors
that should nearly never happen. On the other hand I do not want to hunt
Heisenbugs.
This commit fixes loads of crates (47!) had unused dependencies, or
mis-configured dependencies (test deps as normal deps).
I added the "unused_crate_dependencies" to all crates to help prevent
this mess from growing again!
https://doc.rust-lang.org/beta/nightly-rustc/rustc_lint_defs/builtin/static.UNUSED_CRATE_DEPENDENCIES.html
This has the minor downside of false-positives when specifying
dev-dependencies for test/bench binaries - these are files in /test or
/benches (not normal tests). This commit includes a workaround,
importing them in lib.rs (gated by a feature flag). I think the
trade-off of better dependency management is worth it!
* refactor: avoid `tokio::spawn` for cache requests
During the happy-path, we don't need any tokio task to drive a cache
loader requests because the future issuing the request just act as a
driver. If that future is cancelled, we place the cache request in an
extra task. This avoid latencies due to task overhead and (task) context
switches for most requests. This may remove a millisecond or two from
latency but also makes the whole thing easier to analyze/profile because
we don't spawn a truckload of tasks.
This trick was borrowed from rskafka.
* refactor: split up code
* feat: rework cache refresh logic
Instead of issuing a single refresh when a GET request for a cached key
comes in, start a background job (using some efficient logic to not
overload tokio) per key that refreshes the key using some exponential
backoff. The timer is reset a new GET request comes in. This has the
following advantages:
- our backoff logic decorrelates the requests
- the longer a key was not used, the less often it will be updated
All test (esp. integration tests) as adjusted accordingly, mostly to
account for the fact that no extra GET is required to start the refresh
timer.
Closes#5720.
* docs: improve
Co-authored-by: Andrew Lamb <alamb@influxdata.com>
* refactor: simplify rng overwrite
Co-authored-by: Andrew Lamb <alamb@influxdata.com>
Policy subscribers basically never store `V` (the cached value), so we
should not clone that one unconditionally. But even `K` (the cache key)
is not stored in all cases. So let's pass a reference for both and let
the policies decide if they wanna clone the data or not.
* test: improve test assertion
* refactor: LRU SETs should NOT be treated as "used"
Improves interaction of refresh and LRU.
* docs: improve
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor: improve consistent access under "remove if"
With all the concurrency introduced in #5668, we should be a bit more
careful with our "remove if" handling, esp. if a removal is triggered
while a load is running concurrently. This change introduces as
`remove_if_and_get` helper that ensures this and the querier over to use
it. The parquet file and tombstone caches required a bit of a larger
change because there the invalidation and the actual GET were kinda
separate. We had this separation for the other caches as well at some
point and decided that this easily leads to API misuse, so I took this
opportunity to "fix" the parquet file and tombstone cache as well.
* docs: improve
* refactor: use concrete backend type in `ChangeRequest`
* refactor: "remove if"-checks shall NOT count as "used"
* test: improve docs
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Criterion comes with some extra cargo tooling called `cargo criterion`
which can be used instead of `cargo bench`. The advantage is that we
don't need to compile the entire reporting infrastructure into our
benchmarks. So let's embrace this separation of concerns.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* feat: naive `AddresableHeap::update_order`
* refactor: use `update_order` within LRU policy
* test: add benchmark for `AddressableHeap::update_order`
* refactor: avoid double-hash when updating addressable heap orders
```text
update_order_existing_after_n_elements/1
time: [25.483 ns 25.513 ns 25.547 ns]
change: [-42.490% -42.365% -42.247%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
update_order_existing_after_n_elements/10
time: [68.158 ns 68.211 ns 68.266 ns]
change: [-19.391% -19.131% -18.952%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
update_order_existing_after_n_elements/100
time: [128.10 ns 128.43 ns 128.83 ns]
change: [-17.732% -17.531% -17.255%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe
update_order_existing_after_n_elements/1000
time: [223.08 ns 224.06 ns 225.30 ns]
change: [-9.0635% -8.5828% -7.9794%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
update_order_existing_after_n_elements/10000
time: [1.0032 µs 1.0216 µs 1.0402 µs]
change: [-6.0920% -3.7038% -1.0826%] (p = 0.01 < 0.05)
Performance has improved.
update_order_new_after_n_elements/0
time: [35.898 ns 35.919 ns 35.943 ns]
change: [+183.39% +183.77% +184.12%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
update_order_new_after_n_elements/1
time: [13.273 ns 13.299 ns 13.344 ns]
change: [-6.6980% -5.9798% -5.2633%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe
update_order_new_after_n_elements/10
time: [14.010 ns 14.084 ns 14.183 ns]
change: [-13.579% -13.117% -12.553%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) high mild
9 (9.00%) high severe
update_order_new_after_n_elements/100
time: [23.846 ns 23.883 ns 23.921 ns]
change: [-4.7412% -4.3738% -4.0715%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild
update_order_new_after_n_elements/1000
time: [28.590 ns 28.646 ns 28.705 ns]
change: [-4.1597% -3.6132% -3.0701%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild
update_order_new_after_n_elements/10000
time: [31.459 ns 31.975 ns 32.601 ns]
change: [-32.153% -20.689% -11.961%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
5 (5.00%) high mild
2 (2.00%) high severe
````
Improvements might be even bigger for more expensive hash functions
(e.g. for `K = Arc<str>`).
Note that there is one outlier: `update_order_new_after_n_elements/0`. I
suspect this is due to slightly different compiler decisions (there is
no technical difference for "update a key of an empty heap"). Since this
case is also pretty uncommon in practice (only ~once when the process
boots up), I deem this acceptable.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Instead of a naive "remove + insert", use a proper insertion routine
that touches the hash map only once.
Note that in case of an override (i.e. the entry with this key already
existed) we need to touch the heap twice, because the sort order likely
changed (we don't optimize the "identical order" case here because it is
pretty unlikely that this will happen in practice).
**Perf results:**
```text
insert_n_elements/0 time: [16.489 ns 16.497 ns 16.506 ns]
change: [-8.1154% -7.9967% -7.8990%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severe
insert_n_elements/1 time: [59.806 ns 59.839 ns 59.875 ns]
change: [-14.241% -14.160% -14.086%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
insert_n_elements/10 time: [601.58 ns 602.26 ns 603.09 ns]
change: [-20.870% -20.714% -20.565%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe
insert_n_elements/100 time: [6.9096 µs 6.9161 µs 6.9246 µs]
change: [-18.759% -18.667% -18.553%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe
insert_n_elements/1000 time: [107.71 µs 107.76 µs 107.82 µs]
change: [-14.564% -14.427% -14.295%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
insert_n_elements/10000 time: [2.8642 ms 2.8700 ms 2.8765 ms]
change: [-11.079% -10.860% -10.605%] (p = 0.00 < 0.05)
Performance has improved.
Found 15 outliers among 100 measurements (15.00%)
15 (15.00%) high severe
````
Note that the results are even better for keys with more expansive hash
functions (we have few in the querier).
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* ci: use same feature set in `build_dev` and `build_release`
* ci: also enable unstable tokio for `build_dev`
* chore: update tokio to 1.21 (to fix console-subscriber 0.1.8
* fix: "must use"
This is similar to #5530. Do not box the function because the user will
likely use it as `Box<dyn ResourceEstimator<...>>` or
`Arc<dyn ResourceEstimator<...>>`.
The slightly larger code change is because the clean-up around `TestSize`
which helps with the doctests (and also we had two copies of that type).
There's no technical reason to dyn-dispatch the loader. The user may
free to do so however.
Note that there's no change within `querier` because the type
inference will now automatically treat the passed `Arc` as `Arc<L>`
insteado of `Arc<dyn Loader<...>>`.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Marco Neumann
Carol (Nichols || Goulding)
dependabot[bot]
Stuart Carnie
Dom Dwyer
Andrew Lamb