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

Off-strategy compaction leaks input sstables into "cleanup set" #14304

Closed
raphaelsc opened this issue Jun 19, 2023 · 4 comments · Fixed by #14858
Closed

Off-strategy compaction leaks input sstables into "cleanup set" #14304

raphaelsc opened this issue Jun 19, 2023 · 4 comments · Fixed by #14858
Assignees
@raphaelsc @bhalevy
Labels
area/compaction P2 High Priority type/bug
Milestone
5.4

Comments

@raphaelsc
Copy link
Member

Today, compaction manager has a cleanup set marking all sstables requiring cleanup. Maintenance sstables can be included in this set. Turns out off-strategy compaction is not removing input sstables on completion, so they are leaked, causing additional write amplification as cleanup may pick them afterwards, and space issues as the space of those sstables aren't released till next cleanup happens.
@raphaelsc
Copy link
Member Author

@bhalevy assigned this to you, but I can pick it up if you need help.

@bhalevy
Copy link
Member

bhalevy commented Jun 19, 2023

@bhalevy assigned this to you, but I can pick it up if you need help.

thanks. looks like you got a firm handle on this issue, so feel free to send a fix

@raphaelsc
Copy link
Member Author

@bhalevy assigned this to you, but I can pick it up if you need help.

thanks. looks like you got a firm handle on this issue, so feel free to send a fix

Roger that.

bhalevy added a commit to bhalevy/scylla that referenced this issue Jul 27, 2023
@bhalevy
compaction_manager: on_compaction_completion: erase sstables from sst…
023ed79 
…ables_requiring_cleanup

Move the management of compaction_state::sstables_requiring_cleanup
from compacting_sstable_registration to on_compaction_completion
so it will be performed for all compaction types, as those sstables
were compacted and about to be deleted.

Adjust cleanup_incremental_compaction_test to check
that cleanup doesn't attempt to cleanup already-deleted
sstables that were left over by offstrategy compaction
in sstables_requiring_cleanup.

Fixes scylladb#14304

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
@bhalevy bhalevy mentioned this issue Jul 27, 2023
Merged
bhalevy added a commit to bhalevy/scylla that referenced this issue Jul 27, 2023
@bhalevy
compaction_manager: on_compaction_completion: erase sstables from sst…
43733bf 
…ables_requiring_cleanup

Move the management of compaction_state::sstables_requiring_cleanup
from compacting_sstable_registration to on_compaction_completion
so it will be performed for all compaction types, as those sstables
were compacted and about to be deleted.

Adjust cleanup_incremental_compaction_test to check
that cleanup doesn't attempt to cleanup already-deleted
sstables that were left over by offstrategy compaction
in sstables_requiring_cleanup.

Fixes scylladb#14304

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy added a commit to bhalevy/scylla that referenced this issue Jul 30, 2023
@bhalevy
compaction_manager: on_compaction_completion: erase sstables from sst…
ee75c09 
…ables_requiring_cleanup

Erase retired sstable from compaction_state::sstables_requiring_cleanup
also on_compaction_completion (in addition to
compacting_sstable_registration::release_compacting
for offstrategy compaction with piggybacked cleanup
or any other compaction type that doesn't use
compacting_sstable_registration.

Add cleanup_during_offstrategy_incremental_compaction_test
that is modeled after cleanup_incremental_compaction_test to check
that cleanup doesn't attempt to cleanup already-deleted
sstables that were left over by offstrategy compaction
in sstables_requiring_cleanup.

Fixes scylladb#14304

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
denesb added a commit that referenced this issue Aug 9, 2023
@denesb
Merge 'Update sstable_requiring_cleanup on compaction completion' fro…
108e510 
…m Benny Halevy

Currently `sstable_requiring_cleanup` is updated using `compacting_sstable_registration`, but that mechanism is not used by offstrategy compaction, leading to #14304.

This series introduces `compaction_manager::on_compaction_completion` that intercepts the call
to the table::on_compaction_completion. This allows us to update `sstable_requiring_cleanup` right before the compacted sstables are deleted, making sure they are no leaked to `sstable_requiring_cleanup`, which would hold a reference to them until cleanup attempts to clean them up.

`cleanup_incremental_compaction_test` was adjusted to observe the sstables `on_delete` (by adding a new observer event) to detect the case where cleanup attempts to delete the leaked sstables and fails since they were already deleted from the file system by offstrategy compaction. The test fails with the fix and passes with it.

Fixes #14304

Closes #14858

* github.com:scylladb/scylladb:
  compaction_manager: on_compaction_completion: erase sstables from sstables_requiring_cleanup
  compaction/leveled_compaction_strategy: ideal_level_for_input: special case max_sstable_size==0
  sstable: add on_delete observer
  compaction_manager: add on_compaction_completion
  sstable_compaction_test: cleanup_incremental_compaction_test: verify sstables_requiring_cleanup is empty
@mykaul mykaul added the P2 High Priority label Aug 16, 2023
@bhalevy
Copy link
Member

bhalevy commented Aug 16, 2023 •
edited

sstables_requiring_cleanup exists only in master (and 5.3 that was canceled), but not in 5.2, so no backport is required.

@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
Merge 'Update sstable_requiring_cleanup on compaction completion' fro…
1d4e782 
…m Benny Halevy

Currently `sstable_requiring_cleanup` is updated using `compacting_sstable_registration`, but that mechanism is not used by offstrategy compaction, leading to scylladb#14304.

This series introduces `compaction_manager::on_compaction_completion` that intercepts the call
to the table::on_compaction_completion. This allows us to update `sstable_requiring_cleanup` right before the compacted sstables are deleted, making sure they are no leaked to `sstable_requiring_cleanup`, which would hold a reference to them until cleanup attempts to clean them up.

`cleanup_incremental_compaction_test` was adjusted to observe the sstables `on_delete` (by adding a new observer event) to detect the case where cleanup attempts to delete the leaked sstables and fails since they were already deleted from the file system by offstrategy compaction. The test fails with the fix and passes with it.

\Fixes scylladb#14304

\Closes scylladb#14858

* github.com:scylladb/scylladb:
  compaction_manager: on_compaction_completion: erase sstables from sstables_requiring_cleanup
  compaction/leveled_compaction_strategy: ideal_level_for_input: special case max_sstable_size==0
  sstable: add on_delete observer
  compaction_manager: add on_compaction_completion
  sstable_compaction_test: cleanup_incremental_compaction_test: verify sstables_requiring_cleanup is empty

(cherry picked from commit 108e510)
bhalevy added a commit to bhalevy/scylla that referenced this issue Sep 19, 2023
@bhalevy
Merge 'Update sstable_requiring_cleanup on compaction completion' fro…
9b8a561 
…m Benny Halevy

Currently `sstable_requiring_cleanup` is updated using `compacting_sstable_registration`, but that mechanism is not used by offstrategy compaction, leading to scylladb#14304.

This series introduces `compaction_manager::on_compaction_completion` that intercepts the call
to the table::on_compaction_completion. This allows us to update `sstable_requiring_cleanup` right before the compacted sstables are deleted, making sure they are no leaked to `sstable_requiring_cleanup`, which would hold a reference to them until cleanup attempts to clean them up.

`cleanup_incremental_compaction_test` was adjusted to observe the sstables `on_delete` (by adding a new observer event) to detect the case where cleanup attempts to delete the leaked sstables and fails since they were already deleted from the file system by offstrategy compaction. The test fails with the fix and passes with it.

\Fixes scylladb#14304

\Closes scylladb#14858

* github.com:scylladb/scylladb:
  compaction_manager: on_compaction_completion: erase sstables from sstables_requiring_cleanup
  compaction/leveled_compaction_strategy: ideal_level_for_input: special case max_sstable_size==0
  sstable: add on_delete observer
  compaction_manager: add on_compaction_completion
  sstable_compaction_test: cleanup_incremental_compaction_test: verify sstables_requiring_cleanup is empty

(cherry picked from commit 108e510)
bhalevy added a commit to bhalevy/scylla that referenced this issue Nov 13, 2023
@bhalevy
Merge 'Update sstable_requiring_cleanup on compaction completion' fro…
86a64ee 
…m Benny Halevy

Currently `sstable_requiring_cleanup` is updated using `compacting_sstable_registration`, but that mechanism is not used by offstrategy compaction, leading to scylladb#14304.

This series introduces `compaction_manager::on_compaction_completion` that intercepts the call
to the table::on_compaction_completion. This allows us to update `sstable_requiring_cleanup` right before the compacted sstables are deleted, making sure they are no leaked to `sstable_requiring_cleanup`, which would hold a reference to them until cleanup attempts to clean them up.

`cleanup_incremental_compaction_test` was adjusted to observe the sstables `on_delete` (by adding a new observer event) to detect the case where cleanup attempts to delete the leaked sstables and fails since they were already deleted from the file system by offstrategy compaction. The test fails with the fix and passes with it.

\Fixes scylladb#14304

\Closes scylladb#14858

* github.com:scylladb/scylladb:
  compaction_manager: on_compaction_completion: erase sstables from sstables_requiring_cleanup
  compaction/leveled_compaction_strategy: ideal_level_for_input: special case max_sstable_size==0
  sstable: add on_delete observer
  compaction_manager: add on_compaction_completion
  sstable_compaction_test: cleanup_incremental_compaction_test: verify sstables_requiring_cleanup is empty

(cherry picked from commit 108e510)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@raphaelsc raphaelsc

@bhalevy bhalevy

Labels
area/compaction P2 High Priority type/bug
Projects
None yet
Milestone
5.4
Development

Successfully merging a pull request may close this issue.

Update sstable_requiring_cleanup on compaction completion bhalevy/scylla
5 participants
@raphaelsc @mykaul @bhalevy @scylladb-promoter @DoronArazii