Improve scan-and-compare GC metadata cache - #4838
Open
gaozhangmin wants to merge 2 commits into
Open
gaozhangmin wants to merge 2 commits into
gaozhangmin wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Pull request overview
This PR introduces a ZooKeeper-backed “ledger metadata bucket” watch/cache in LedgerManager and updates scan-and-compare GC to use that cache to avoid walking all metadata ranges, aiming to significantly reduce GC metadata overhead on large deployments.
Changes:
- Adds a ledger-metadata bucket cache API to LedgerManager and a ZooKeeper implementation in AbstractZkLedgerManager.
- Updates scan-and-compare GC to consult the bucket cache (and adds a force GC signal plumbed through GarbageCollectorThread).
- Updates local storage implementations to notify the ledger manager when a ledger is first observed locally, and adds/adjusts tests for cache behavior and GC timing.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file| File | Description |
|---|---|
| bookkeeper-server/src/test/java/org/apache/bookkeeper/util/TestZkUtils.java | Adds ZooKeeper watch-path behavior tests. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/test/LedgerDeleteTest.java | Replaces fixed sleeps with Awaitility-based waiting for entry log deletion. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java | Updates GC tests for new GC signature/cache behavior; adds new cache-focused tests. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerTest.java | Adds unit tests for bucket-cache refresh/stale/absent/pruning behavior. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java | Adds tests for local-ledger notification behavior in Db storage. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java | Adds tests for local-ledger notification behavior in sorted storage. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java | Makes compaction-time assertion less flaky (>= vs >). |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java | Adds metadata bucket cache API (default methods + result enum). |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java | Delegates new cache-related LedgerManager methods to the underlying manager. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java | Implements the ZooKeeper-backed bucket watch/cache and refresh logic. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java | Notifies ledger manager on first local setMasterKey per ledger. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java | Uses metadata bucket cache during scan-and-compare GC; adds force mode. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java | Notifies ledger manager on first local setMasterKey per ledger. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java | Plumbs “force GC” into the ledger GC step. |
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollector.java | Adds a default gc(..., boolean force) overload for backward compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
Comment thread
...keeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
Comment on lines
+255
to
+275
| for (Long bkLid : bkActiveLedgers) { | ||
| ledgerManager.ensureLedgerMetadataBucketWatched(bkLid); | ||
| LedgerMetadataCacheResult cacheResult = ledgerManager.lookupLedgerMetadataInCache(bkLid); | ||
| if (cacheResult == LedgerMetadataCacheResult.UNKNOWN) { | ||
| continue; | ||
| } | ||
| if (cacheResult == LedgerMetadataCacheResult.PRESENT && !verifyMetadataOnGc) { | ||
| continue; | ||
| } | ||
| if (cacheResult == LedgerMetadataCacheResult.PRESENT && !isBookieMissingFromMetadata(bkLid, | ||
| zkOpTimeoutMs)) { | ||
| continue; | ||
| } | ||
| if (cacheResult == LedgerMetadataCacheResult.MISSING | ||
| && verifyMetadataOnGc && !isBookieMissingFromMetadata(bkLid, zkOpTimeoutMs)) { | ||
| continue; | ||
| } | ||
| if (cacheResult == LedgerMetadataCacheResult.MISSING || verifyMetadataOnGc) { | ||
| garbageCleaner.clean(bkLid); | ||
| } | ||
| } |
Comment thread
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
Comment on lines
99
to
+101
| protected final SnapshotMap<Long, Boolean> activeLedgers; | ||
| private LedgerManager ledgerManager; | ||
| private final Set<Long> notifiedLocalLedgers = ConcurrentHashMap.newKeySet(); |
Comment thread
...r/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java
Comment on lines
+155
to
+156
| private final LedgerManager ledgerManager; | ||
| private final Set<Long> notifiedLocalLedgers = ConcurrentHashMap.newKeySet(); |
Comment on lines
+104
to
+110
| zkc.getChildren(parentPath, new Watcher() { | ||
| @Override | ||
| public void process(WatchedEvent event) { | ||
| eventRef.set(event); | ||
| eventReceived.countDown(); | ||
| } | ||
| }); |
Comment on lines
+130
to
+136
| zkc.exists(childPath, new Watcher() { | ||
| @Override | ||
| public void process(WatchedEvent event) { | ||
| eventRef.set(event); | ||
| eventReceived.countDown(); | ||
| } | ||
| }); |
Comment thread
...eeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java
Comment on lines
+267
to
+270
| } finally { | ||
| dbStorage.shutdown(); | ||
| dbTmpDir.delete(); | ||
| } |
This file contains hidden or 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
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.
Descriptions of the changes in this PR:
Fixes: N/A
Main Issue: N/A
BP: N/A
Motivation
Scan-and-compare GC currently walks all ledger metadata ranges from ZooKeeper to decide whether locally active ledgers still exist in metadata. On deployments with many ledgers, fetching every ledger range can make ScanAndCompareGarbageCollector.gc() spend minutes in metadata operations even when the bookie only needs to check a small subset of local ledger buckets.
Changes
Verifications