compaction: Fix sstable cleanup after resharding on refresh #14147
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
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
|
CI state |
bhalevy
reviewed
Jun 6, 2023
| @@ -1590,6 +1590,10 @@ bool needs_cleanup(const sstables::shared_sstable& sst, | |||
|
|
|||
| bool compaction_manager::update_sstable_cleanup_state(table_state& t, const sstables::shared_sstable& sst, const dht::token_range_vector& sorted_owned_ranges) { | |||
| auto& cs = get_compaction_state(&t); | |||
| if (sst->is_shared()) { | |||
| throw std::runtime_error(format("Shared SSTable {} cannot be marked as requiring cleanup, as it can only be processed by resharding", | |||
| sst->get_filename())); | |||
There was a problem hiding this comment.
nit: we now have a printer for shared_sstable that prints also the sstable origin, that might be relevant here, so better pass just sst.
bhalevy
reviewed
Jun 6, 2023
| co_await d.filter_sstables([&] (sstables::shared_sstable sst) -> future<bool> { | ||
| if (table.update_sstable_cleanup_state(sst, *sorted_owned_ranges_ptr)) { | ||
| if (needs_cleanup(sst, *sorted_owned_ranges_ptr)) { |
There was a problem hiding this comment.
BTW, the reason this wasn't the root cause for #14001 is that here we filter the unshared sstables, meaning it's benign to cleanup those sstables later on from the multi-shard sharing perspective.
But the hunk is still needed.
The original intent was to detect sstables requiring cleanup on start-up, when populating the table.
But we're not there yet as we don't have the token metadata in this early stage.
bhalevy
requested changes
Jun 6, 2023
bhalevy
pushed a commit
to bhalevy/scylla
that referenced
this pull request
Jun 11, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#14147 (cherry picked from commit 156d771) Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
denesb
pushed a commit
that referenced
this pull request
Jun 13, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes #14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes #14147 (cherry picked from commit 156d771)
bhalevy
pushed a commit
to bhalevy/scylla
that referenced
this pull request
Jun 14, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#14147 (cherry picked from commit 156d771) Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
pushed a commit
to bhalevy/scylla
that referenced
this pull request
Aug 31, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#14147 (cherry picked from commit 156d771) Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
pushed a commit
to bhalevy/scylla
that referenced
this pull request
Sep 19, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#14147 (cherry picked from commit 156d771) Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
pushed a commit
to bhalevy/scylla
that referenced
this pull request
Nov 13, 2023
Problem can be reproduced easily: 1) wrote some sstables with smp 1 2) shut down scylla 3) moved sstables to upload 4) restarted scylla with smp 2 5) ran refresh (resharding happens, adds sstable to cleanup set and never removes it) 6) cleanup (tries to cleanup resharded sstables which were leaked in the cleanup set) Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding. Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files). Dtest: scylladb/scylla-dtest#3206 Fixes scylladb#14001. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#14147 (cherry picked from commit 156d771) Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
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.
Problem can be reproduced easily:
Bumps into assert "Assertion `!sst->is_shared()' failed", as cleanup picks a shared sstable that was leaked and already processed by resharding.
Fix is about not inserting shared sstables into cleanup set, as shared sstables are restricted to resharding and cannot be processed later by cleanup (nor it should because resharding itself cleaned up its input files).
Dtest: https://github.com/scylladb/scylla-dtest/pull/3206
Fixes #14001.