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.

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

Fix SST files not being cleaned up in the locations folder #4555

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danpi
Copy link

@danpi danpi commented Feb 20, 2025

Descriptions of the changes in this PR:

Fix #4554

Motivation

Resolve issue with SST files remaining uncleared in the locations folder.

Changes

  1. Reuse the cleanup logic of GarbageCollectorThread to trigger entryLocationCompact() when a major compaction occurs.
  2. Add the entryLocationCompaction configuration option to control the above behavior, disabled by default.

@danpi
Copy link
Author

danpi commented Feb 20, 2025

I believe this issue is related to optimization #3653.

When using deleteRange to replace the one-by-one deletion method, only one action is written to RocksDB. If RocksDB is performing tiered compaction, this action is not guaranteed to be picked up, causing SST files to remain undeleted.

To resolve this issue:
Option 1: Revert to the previous logic of iteratively deleting each key.
Option 2: Use compactRange to forcefully clean up levels L0 to L6.
I chose to perform a compactRange synchronously when executing a major compaction.

@hangc0276
Copy link
Contributor

hangc0276 commented Feb 27, 2025

I believe this issue is related to optimization #3653.

When using deleteRange to replace the one-by-one deletion method, only one action is written to RocksDB. If RocksDB is performing tiered compaction, this action is not guaranteed to be picked up, causing SST files to remain undeleted.

To resolve this issue: Option 1: Revert to the previous logic of iteratively deleting each key. Option 2: Use compactRange to forcefully clean up levels L0 to L6. I chose to perform a compactRange synchronously when executing a major compaction.

@danpi Great job! I think it should be a bug in RocksDB. We can use option 2. Please check the failed CI, thanks.

@danpi
Copy link
Author

danpi commented Mar 6, 2025

@hangc0276 Addressed the feedback above. PTAL, thanks.

@@ -87,6 +87,10 @@ public class GarbageCollectorThread implements Runnable {
long majorCompactionMaxTimeMillis;
long lastMajorCompactionTime;

boolean enableEntryLocationCompaction = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this one and use entryLocationCompactionInterval > 0 instead?

@@ -489,6 +505,17 @@ public void runWithFlags(boolean force, boolean suspendMajor, boolean suspendMin
minorCompacting.set(false);
}
}
if (enableEntryLocationCompaction && (curTime - lastEntryLocationCompactionTime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd better introduce a random factor for it. If we roll restart the BookKeeper cluster, and all the bookies start time are the same, which will make all the bookies triggered RocksDB compaction at the same time and impact the read latency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The random factor can be any time in [0, entryLocationCompactionInterval]

@hangc0276
Copy link
Contributor

@hangc0276 Addressed the feedback above. PTAL, thanks.

@danpi Thanks for your contribution. I left more comments; please take a look. Thanks.

@danpi
Copy link
Author

danpi commented Mar 10, 2025

@hangc0276 Addressed the feedback above. PTAL, thanks.

@danpi Thanks for your contribution. I left more comments; please take a look. Thanks.

@hangc0276 Add randomCompactionDelay to avoid all the bookies triggering compaction simultaneously. PTAL, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]Large number of SST files in locations not being cleared after upgrade
2 participants