Tiered compaction: cut deltas along lsn as well if needed #7671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@hlinnaka could you give this PR a look? It hits an infinite loop issue if you run it like: Basically, the main compaction loop never terminates and there is output like: I suppose we'd need to change the |
3060 tests run: 2927 passed, 0 failed, 133 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
e715cd6 at 2024-05-13T22:25:56.791Z :recycle: |
arpad-m
force-pushed
the
arpad/compaction_split_delta_lsn
branch
from
e457d8a
to
e6fa290
Compare
arpad-m
changed the title
arpad-m
force-pushed
the
arpad/compaction_split_delta_lsn
branch
from
e6fa290
to
00942ab
Compare
|
Requesting review by @hlinnaka as Christian isn't available on Friday. |
hlinnaka
added a commit
that referenced
this pull request
May 13, 2024
- Replace 'drain_window' closure with 'create_delta_job'. That can then also be used in the loop that creates the delta jobs for the single key - Use a match-statement to handle the three cases: end of keyspace, "normal case", and single large key
hlinnaka
reviewed
May 13, 2024
There was a problem hiding this comment.
Window::feed function has code to deal with the case that you feed it the same key multiple times, but that seems to be dead code now. Maybe remove it and replace with an assertion that the new key is always > previous one
While reviewing this, I did some further refactoring to help me understand this better: #7724. Hope you find it useful.
- Replace 'drain_window' closure with 'create_delta_job'. That can then also be used in the loop that creates the delta jobs for the single key - Use a match-statement to handle the three cases: end of keyspace, "normal case", and single large key
Good point, but I think this has always been dead code: Thanks for #7724, the code is clearer now. Merged. |
arpad-m
force-pushed
the
arpad/compaction_split_delta_lsn
branch
from
297cd5f
to
ac5d72a
Compare
problame
approved these changes
May 13, 2024
|
|
hlinnaka
approved these changes
May 13, 2024
|
Merged, will file PR for the test as a followup (plus PR for the edit:
|
arpad-m
added a commit
that referenced
this pull request
May 16, 2024
Tiered compaction employs two sliding windows over the keyspace: `KeyspaceWindow` for the image layer generation and `Window` for the delta layer generation. Do some fixes to both windows: * The distinction between the two windows is not very clear. Do the absolute minimum to mention where they are used in the rustdoc description of the struct. Maybe we should rename them (say `WindowForImage` and `WindowForDelta`) or merge them into one window implementation. * Require the keys to strictly increase. The `accum_key_values` already combines the key, so there is no logic needed in `Window::feed` for the same key repeating. This is a follow-up to address the request in #7671 (review) * In `choose_next_delta`, we claimed in the comment to use 1.25 as the factor but it was 1.66 instead. Fix this discrepancy by using `*5/4` as the two operations.
a-masterov
pushed a commit
that referenced
this pull request
May 20, 2024
In general, tiered compaction is splitting delta layers along the key dimension, but this can only continue until a single key is reached: if the changes from a single key don't fit into one layer file, we used to create layer files of unbounded sizes. This patch implements the method listed as TODO/FIXME in the source code. It does the following things: * Make `accum_key_values` take the target size and if one key's modifications exceed it, make it fill `partition_lsns`, a vector of lsns to use for partitioning. * Have `retile_deltas` use that `partition_lsns` to create delta layers separated by lsn. * Adjust the `test_many_updates_for_single_key` to allow layer files below 0.5 the target size. This situation can create arbitarily small layer files: The amount of data is arbitrary that sits between having just cut a new delta, and then stumbling upon the key that needs to be split along lsn. This data will end up in a dedicated layer and it can be arbitrarily small. * Ignore single-key delta layers for depth calculation: in theory we might have only single-key delta layers in a tier, and this might confuse depth calculation as well, but this should be unlikely. Fixes #7243 Part of #7554 --------- Co-authored-by: Heikki Linnakangas <heikki@neon.tech>
a-masterov
pushed a commit
that referenced
this pull request
May 20, 2024
Tiered compaction employs two sliding windows over the keyspace: `KeyspaceWindow` for the image layer generation and `Window` for the delta layer generation. Do some fixes to both windows: * The distinction between the two windows is not very clear. Do the absolute minimum to mention where they are used in the rustdoc description of the struct. Maybe we should rename them (say `WindowForImage` and `WindowForDelta`) or merge them into one window implementation. * Require the keys to strictly increase. The `accum_key_values` already combines the key, so there is no logic needed in `Window::feed` for the same key repeating. This is a follow-up to address the request in #7671 (review) * In `choose_next_delta`, we claimed in the comment to use 1.25 as the factor but it was 1.66 instead. Fix this discrepancy by using `*5/4` as the two operations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In general, tiered compaction is splitting delta layers along the key dimension, but this can only continue until a single key is reached: if the changes from a single key don't fit into one layer file, we used to create layer files of unbounded sizes.
This patch implements the method listed as TODO/FIXME in the source code. It does the following things:
accum_key_valuestake the target size and if one key's modifications exceed it, make it fillpartition_lsns, a vector of lsns to use for partitioning.retile_deltasuse thatpartition_lsnsto create delta layers separated by lsn.test_many_updates_for_single_keyto allow layer files below 0.5 the target size. This situation can create arbitarily small layer files: The amount of data is arbitrary that sits between having just cut a new delta, and then stumbling upon the key that needs to be split along lsn. This data will end up in a dedicated layer and it can be arbitrarily small.Fixes #7243
Part of #7554