Copilot commented on code in PR #4801:
URL: https://github.com/apache/bookkeeper/pull/4801#discussion_r3309069315
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java:
##########
@@ -819,6 +819,11 @@ public synchronized void shutdown() throws
InterruptedException {
} catch (Exception e) {
log.warn().exception(e).log("Failed to close entryLog
metadata-map");
}
+ try {
+ garbageCollector.close();
+ } catch (Exception e) {
+ log.warn().exception(e).log("Failed to close garbage collector
metadata resources");
+ }
Review Comment:
This introduces a new lifecycle dependency (`close()`) on
`garbageCollector`. If `garbageCollector` is typed as the `GarbageCollector`
interface (or another type that doesn’t declare `close()`), this either won’t
compile or forces consumers to know the concrete implementation. Consider
making `GarbageCollector` extend `AutoCloseable` (or add a `close()` method to
that interface) so lifecycle management is part of the contract.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java:
##########
@@ -340,6 +352,24 @@ private static MetadataBookieDriver
instantiateMetadataDriver(ServerConfiguratio
}
}
+ @Override
+ public synchronized void close() {
+ closeMetadataDriver();
+ }
+
+ private void closeMetadataDriver() {
+ metadataLedgerManagerFactory = null;
+ if (metadataDriver != null) {
+ try {
+ metadataDriver.close();
+ } catch (Exception e) {
+ log.warn().exception(e).log("Failed to close metadata driver
used for over-replicated ledger GC");
+ } finally {
+ metadataDriver = null;
+ }
+ }
+ }
Review Comment:
The previous implementation explicitly closed the `LedgerManagerFactory` via
`@Cleanup`, but the new cached implementation only closes `metadataDriver` and
nulls `metadataLedgerManagerFactory` without closing it. If
`LedgerManagerFactory` holds resources (as suggested by the earlier `@Cleanup`
usage), this change can leak resources across the process lifetime. Close
`metadataLedgerManagerFactory` (with its own try/catch) before setting it to
null, and then close the driver.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/GarbageCollectorThreadTest.java:
##########
@@ -145,6 +155,46 @@ public void testCalculateUsageBucket() {
Assert.assertEquals("Incorrect number of items", items + 1, sum);
}
+ @Test
+ public void testOverreplicatedLedgerGcReusesMetadataDriverUntilClosed()
throws Exception {
+ ServerConfiguration bkConf =
TestBKConfiguration.newServerConfiguration()
+ .setAllowLoopback(true)
+ .setMetadataServiceUri("zk://127.0.0.1/ledgers");
+ MetadataBookieDriver metadataDriver = mock(MetadataBookieDriver.class);
+ LedgerManagerFactory lmf = mock(LedgerManagerFactory.class);
+ when(metadataDriver.getLedgerManagerFactory()).thenReturn(lmf);
+
+ try (MockedStatic<MetadataDrivers> metadataDrivers =
mockStatic(MetadataDrivers.class)) {
+ metadataDrivers.when(() ->
MetadataDrivers.getBookieDriver(any(URI.class))).thenReturn(metadataDriver);
+ ScanAndCompareGarbageCollector garbageCollector = new
ScanAndCompareGarbageCollector(
+ ledgerManager, ledgerStorage, bkConf,
NullStatsLogger.INSTANCE);
+
+ Assert.assertSame(lmf,
getOrCreateMetadataLedgerManagerFactory(garbageCollector));
+ Assert.assertSame(lmf,
getOrCreateMetadataLedgerManagerFactory(garbageCollector));
+ metadataDrivers.verify(() ->
MetadataDrivers.getBookieDriver(any(URI.class)), times(1));
+
+ garbageCollector.close();
+
+ verify(metadataDriver).close();
+ Assert.assertNull(getMetadataDriver(garbageCollector));
+ }
+ }
+
+ private static LedgerManagerFactory
getOrCreateMetadataLedgerManagerFactory(
+ ScanAndCompareGarbageCollector garbageCollector) throws Exception {
+ Method method = ScanAndCompareGarbageCollector.class.getDeclaredMethod(
+ "getOrCreateMetadataLedgerManagerFactory");
+ method.setAccessible(true);
+ return (LedgerManagerFactory) method.invoke(garbageCollector);
+ }
+
+ private static MetadataBookieDriver
getMetadataDriver(ScanAndCompareGarbageCollector garbageCollector)
+ throws Exception {
+ Field field =
ScanAndCompareGarbageCollector.class.getDeclaredField("metadataDriver");
+ field.setAccessible(true);
+ return (MetadataBookieDriver) field.get(garbageCollector);
+ }
Review Comment:
The test relies on deep reflection (`setAccessible(true)`) into private
members/methods, which is brittle under JPMS/strong encapsulation and makes
refactors harder. Prefer asserting behavior through a test seam (e.g., inject a
`MetadataDriverFactory`/supplier into `ScanAndCompareGarbageCollector`, or
expose package-private accessors in the same package) so the test verifies
caching/close behavior without reflective access.
--
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]
