Copilot commented on code in PR #4838:
URL: https://github.com/apache/bookkeeper/pull/4838#discussion_r3570335294
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -150,6 +152,8 @@ protected Thread newThread(Runnable r, String name) {
private final boolean singleLedgerDirs;
private final String ledgerBaseDir;
private final String indexBaseDir;
+ private final LedgerManager ledgerManager;
+ private final Set<Long> notifiedLocalLedgers =
ConcurrentHashMap.newKeySet();
Review Comment:
notifiedLocalLedgers is an unbounded set that grows with every distinct
ledger ever seen, and it is never pruned. On bookies with high ledger churn
this can lead to long-lived heap growth and potential OOM over time.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/util/TestZkUtils.java:
##########
@@ -85,4 +90,56 @@ public void testAsyncCreateAndDeleteFullPathOptimistic()
throws IOException, Kee
assertTrue("/ledgers/000" + " zNode should not exist, since it should
be deleted recursively",
null == zkc.exists(ledgerZnodePath, false));
}
+
+ @Test
+ public void testParentChildrenWatchReceivesParentPathWhenChildDeleted()
throws Exception {
+ ZooKeeper zkc = zkUtil.getZooKeeperClient();
+ String parentPath = "/parent-watch-test";
+ String childPath = parentPath + "/child";
+ zkc.create(parentPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+ zkc.create(childPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+
+ CountDownLatch eventReceived = new CountDownLatch(1);
+ AtomicReference<WatchedEvent> eventRef = new AtomicReference<>();
+ zkc.getChildren(parentPath, new Watcher() {
+ @Override
+ public void process(WatchedEvent event) {
+ eventRef.set(event);
+ eventReceived.countDown();
+ }
+ });
Review Comment:
This watcher counts down on the first WatchedEvent, but ZooKeeper watchers
can receive session events (EventType.None) or other non-children-change
events, which can make this test flaky and/or assert on a null/irrelevant path.
Filter for the expected EventType/path before counting down.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java:
##########
@@ -230,6 +242,80 @@ public void gc(GarbageCleaner garbageCleaner) {
}
}
+ private boolean gcLedgersUsingMetadataCache(NavigableSet<Long>
bkActiveLedgers, GarbageCleaner garbageCleaner,
+ long zkOpTimeoutMs, boolean
force) throws Exception {
+ if (!ledgerManager.supportsLedgerMetadataCache()) {
+ return false;
+ }
+
+ if (force) {
+
ledgerManager.refreshLedgerMetadataBucketsForLedgers(bkActiveLedgers);
+ }
+
+ 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);
+ }
+ }
Review Comment:
In the metadata-cache path, when verifyMetadataOnGc is enabled the code
calls readLedgerMetadata() even when the cache reports PRESENT, and can end up
calling garbageCleaner.clean() based on ensemble membership. This deviates from
the previous scan-and-compare behavior (which only verified metadata for
ledgers missing from the metadata listing), can introduce a large per-ledger
metadata read load, and may delete ledgers without the underreplication/lock
coordination used by the dedicated overreplicated-ledger GC path.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java:
##########
@@ -95,6 +97,8 @@ public class InterleavedLedgerStorage implements
CompactableLedgerStorage, Entry
// A sorted map to stored all active ledger ids
protected final SnapshotMap<Long, Boolean> activeLedgers;
+ private LedgerManager ledgerManager;
+ private final Set<Long> notifiedLocalLedgers =
ConcurrentHashMap.newKeySet();
Review Comment:
notifiedLocalLedgers is an unbounded set that grows with every distinct
ledger ever seen, and it is never pruned. On bookies with high ledger churn
this can lead to long-lived heap growth and potential OOM over time.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/util/TestZkUtils.java:
##########
@@ -85,4 +90,56 @@ public void testAsyncCreateAndDeleteFullPathOptimistic()
throws IOException, Kee
assertTrue("/ledgers/000" + " zNode should not exist, since it should
be deleted recursively",
null == zkc.exists(ledgerZnodePath, false));
}
+
+ @Test
+ public void testParentChildrenWatchReceivesParentPathWhenChildDeleted()
throws Exception {
+ ZooKeeper zkc = zkUtil.getZooKeeperClient();
+ String parentPath = "/parent-watch-test";
+ String childPath = parentPath + "/child";
+ zkc.create(parentPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+ zkc.create(childPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+
+ CountDownLatch eventReceived = new CountDownLatch(1);
+ AtomicReference<WatchedEvent> eventRef = new AtomicReference<>();
+ zkc.getChildren(parentPath, new Watcher() {
+ @Override
+ public void process(WatchedEvent event) {
+ eventRef.set(event);
+ eventReceived.countDown();
+ }
+ });
+
+ zkc.delete(childPath, -1);
+
+ assertTrue("parent children watch should fire",
+ eventReceived.await(10, TimeUnit.SECONDS));
+ assertEquals(Watcher.Event.EventType.NodeChildrenChanged,
eventRef.get().getType());
+ assertEquals(parentPath, eventRef.get().getPath());
+ }
+
+ @Test
+ public void testChildExistsWatchReceivesChildPathWhenChildDeleted() throws
Exception {
+ ZooKeeper zkc = zkUtil.getZooKeeperClient();
+ String parentPath = "/child-watch-test";
+ String childPath = parentPath + "/child";
+ zkc.create(parentPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+ zkc.create(childPath, new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+
+ CountDownLatch eventReceived = new CountDownLatch(1);
+ AtomicReference<WatchedEvent> eventRef = new AtomicReference<>();
+ zkc.exists(childPath, new Watcher() {
+ @Override
+ public void process(WatchedEvent event) {
+ eventRef.set(event);
+ eventReceived.countDown();
+ }
+ });
Review Comment:
This watcher counts down on the first WatchedEvent, but ZooKeeper watchers
can receive session events (EventType.None) or other non-delete events, which
can make this test flaky and/or assert on a null/irrelevant path. Filter for
the expected EventType/path before counting down.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java:
##########
@@ -232,6 +238,38 @@ public void simple() throws Exception {
}
}
+ @Test
+ public void testSetMasterKeyNotifiesLedgerAddedToLocalStorageOnce() throws
Exception {
+ File dbTmpDir = File.createTempFile("bkDbTest", ".dir");
+ dbTmpDir.delete();
+ dbTmpDir.mkdir();
+ File curDir = BookieImpl.getCurrentDirectory(dbTmpDir);
+ BookieImpl.checkDirectoryStructure(curDir);
+
+ ServerConfiguration dbConf =
TestBKConfiguration.newServerConfiguration();
+ dbConf.setGcWaitTime(1000);
+ dbConf.setLedgerStorageClass(DbLedgerStorage.class.getName());
+ dbConf.setLedgerDirNames(new String[] { dbTmpDir.toString() });
+ LedgerDirsManager dbLedgerDirsManager = new LedgerDirsManager(dbConf,
dbConf.getLedgerDirs(),
+ ledgerDirsManager.getDiskChecker());
+ DbLedgerStorage dbStorage = new DbLedgerStorage();
+ LedgerManager ledgerManager = mock(LedgerManager.class);
+ try {
+ dbStorage.initialize(dbConf, ledgerManager, dbLedgerDirsManager,
dbLedgerDirsManager,
+ NullStatsLogger.INSTANCE,
UnpooledByteBufAllocator.DEFAULT);
+
+ dbStorage.setMasterKey(123L, "ledger-123".getBytes());
+ dbStorage.setMasterKey(123L, "ledger-123".getBytes());
+ dbStorage.setMasterKey(124L, "ledger-124".getBytes());
+
+ verify(ledgerManager, times(1)).onLedgerAddedToLocalStorage(123L);
+ verify(ledgerManager, times(1)).onLedgerAddedToLocalStorage(124L);
+ } finally {
+ dbStorage.shutdown();
+ dbTmpDir.delete();
+ }
Review Comment:
dbTmpDir.delete() will not remove a non-empty directory, so this test will
typically leak temp directories/files on disk. Prefer recursive deletion
(consistent with other tests using FileUtils.deleteDirectory).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
