Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Offstrategy compaction must piggyback cleanup on reshape when invoked from the cleanup path #15041

Closed
bhalevy opened this issue Aug 14, 2023 · 6 comments
Closed

Offstrategy compaction must piggyback cleanup on reshape when invoked from the cleanup path #15041

bhalevy opened this issue Aug 14, 2023 · 6 comments
Assignees
@bhalevy
Labels
area/compaction Backport candidate P2 High Priority
Milestone
5.4

Comments

@bhalevy
Copy link
Member

bhalevy commented Aug 14, 2023 •
edited

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

try_perform_clean first scans both the main and maintenance sets,
looking for sstables that require cleanup, and it calls perform_offstrategy
if any sstable in the maintenance set requires cleanup, and it expects offstrategy,
in this case, to piggyback cleanup on reshape compactio, based on the owned_ranges_ptr
try_perform_clean sets up in the compaction_state.

Currently, offstrategy compaction ignores the owned ranges ptr in compaction_state
and so, no cleanup is done during reshape and the reshape output sstable(s)
will contain disowned tokens, which are not cleaned up, since try_cleanup_compaction
does not scan the reshape output sstable(s). If it would, they would be added to
sstables_requiring_cleanup, but this is less efficient, since it will require yet another
pass over the data, while piggybacking cleanup on reshape does that in a single pass.
@bhalevy bhalevy self-assigned this Aug 14, 2023
@mykaul
Copy link
Contributor

mykaul commented Aug 14, 2023

What's the priority/severity of this?

@bhalevy
Copy link
Member Author

bhalevy commented Aug 14, 2023

The severity is high since this may cause data resurrection.
But priority can be P2 IMO since the combination of events is quite rare.
In any case, I'm testing the fix right now, so it doesn't matter much :)
And it doesn't need backporting since it's confined to master and doesn't exist in 5.2 and earlier (that have #9559 instead which is too risky to backport)

@bhalevy bhalevy added the P2 High Priority label Aug 14, 2023
@bhalevy
Copy link
Member Author

bhalevy commented Aug 14, 2023

I'm also improving the test_mv_resurrected_rows_after_decommission_interrupt dtest to reproduce this issue.

bhalevy added a commit to bhalevy/scylla that referenced this issue Aug 14, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
81dd87b 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.  In this case the compaction_state.owned_ranges_ptr
is set, but run_offstrategy_compaction currently ignores
that and doesn't piggyback cleanup on reshape, which
it must.

Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
@bhalevy bhalevy mentioned this issue Aug 14, 2023
Closed
bhalevy added a commit to bhalevy/scylla that referenced this issue Aug 14, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
f9935e7 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

The input sstables are inserted into the compaction_state
`sstables_requiring_cleanup` and `try_perform_cleanup`
expects offstrategy compaction to clean them up along
with reshape compaction.

Otherwise, the maintenance sstables that require cleanup
are not cleaned up by cleanup compaction, since
the reshape output sstable(s) are not analyzed again
after reshape compaction, where that would insert
the output sstable(s) into `sstables_requiring_cleanup`
and trigger their cleanup in the subsequent cleanup compaction.

The latter method is viable too, but it is less effficient
since we can do reshape+cleanup in one pass, vs.
reshape first and cleanup later.

Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
@bhalevy bhalevy changed the title Offstrategy compaction does not piggyback cleanup on reshape when it should Offstrategy compaction must piggyback cleanup on reshape when invoked from the cleanup path Aug 14, 2023
bhalevy added a commit to bhalevy/scylla that referenced this issue Aug 15, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
2b5385d 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

The input sstables are inserted into the compaction_state
`sstables_requiring_cleanup` and `try_perform_cleanup`
expects offstrategy compaction to clean them up along
with reshape compaction.

Otherwise, the maintenance sstables that require cleanup
are not cleaned up by cleanup compaction, since
the reshape output sstable(s) are not analyzed again
after reshape compaction, where that would insert
the output sstable(s) into `sstables_requiring_cleanup`
and trigger their cleanup in the subsequent cleanup compaction.

The latter method is viable too, but it is less effficient
since we can do reshape+cleanup in one pass, vs.
reshape first and cleanup later.

Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Closes scylladb#15043
@bhalevy
Copy link
Member Author

bhalevy commented Aug 28, 2023

Adding Backport candidate label.
I don't know why automation didn't add it...

@DoronArazii DoronArazii added this to the 5.4 milestone Aug 29, 2023
bhalevy added a commit to bhalevy/scylla that referenced this issue Aug 31, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
48fc374 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

The input sstables are inserted into the compaction_state
`sstables_requiring_cleanup` and `try_perform_cleanup`
expects offstrategy compaction to clean them up along
with reshape compaction.

Otherwise, the maintenance sstables that require cleanup
are not cleaned up by cleanup compaction, since
the reshape output sstable(s) are not analyzed again
after reshape compaction, where that would insert
the output sstable(s) into `sstables_requiring_cleanup`
and trigger their cleanup in the subsequent cleanup compaction.

The latter method is viable too, but it is less effficient
since we can do reshape+cleanup in one pass, vs.
reshape first and cleanup later.

\Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

\Closes scylladb#15043

(cherry picked from commit 9f77a32)
bhalevy added a commit to bhalevy/scylla that referenced this issue Sep 19, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
172718f 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

The input sstables are inserted into the compaction_state
`sstables_requiring_cleanup` and `try_perform_cleanup`
expects offstrategy compaction to clean them up along
with reshape compaction.

Otherwise, the maintenance sstables that require cleanup
are not cleaned up by cleanup compaction, since
the reshape output sstable(s) are not analyzed again
after reshape compaction, where that would insert
the output sstable(s) into `sstables_requiring_cleanup`
and trigger their cleanup in the subsequent cleanup compaction.

The latter method is viable too, but it is less effficient
since we can do reshape+cleanup in one pass, vs.
reshape first and cleanup later.

\Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

\Closes scylladb#15043

(cherry picked from commit 9f77a32)
bhalevy added a commit to bhalevy/scylla that referenced this issue Nov 13, 2023
@bhalevy
compaction_manager: run_offstrategy_compaction: retrieve owned_ranges…
e274e96 
… from compaction_state

perform_offstrategy is called from try_perform_cleanup
when there are sstables in the maintenance set that require
cleanup.

The input sstables are inserted into the compaction_state
`sstables_requiring_cleanup` and `try_perform_cleanup`
expects offstrategy compaction to clean them up along
with reshape compaction.

Otherwise, the maintenance sstables that require cleanup
are not cleaned up by cleanup compaction, since
the reshape output sstable(s) are not analyzed again
after reshape compaction, where that would insert
the output sstable(s) into `sstables_requiring_cleanup`
and trigger their cleanup in the subsequent cleanup compaction.

The latter method is viable too, but it is less effficient
since we can do reshape+cleanup in one pass, vs.
reshape first and cleanup later.

\Fixes scylladb#15041

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

\Closes scylladb#15043

(cherry picked from commit 9f77a32)
@denesb
Copy link
Contributor

denesb commented Dec 18, 2023

@bhalevy is this part of the cleanup backport series? If not, please prepare a backport PR agasint 5.2.

@bhalevy
Copy link
Member Author

bhalevy commented Dec 18, 2023

172718f is already part of #14200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bhalevy bhalevy

Labels
area/compaction Backport candidate P2 High Priority
Projects
None yet
Milestone
5.4
4 participants
@denesb @mykaul @bhalevy @DoronArazii