Copilot commented on code in PR #9142:
URL: https://github.com/apache/ozone/pull/9142#discussion_r2426626336
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +449,143 @@ public void testWriteDBToArchive(boolean
expectOnlySstFiles) throws Exception {
}
}
+ @Test
+ public void testBootstrapLockCoordination() throws Exception {
+ // Create mocks for all background services
+ KeyDeletingService mockDeletingService = mock(KeyDeletingService.class);
+ DirectoryDeletingService mockDirDeletingService =
mock(DirectoryDeletingService.class);
+ SstFilteringService mockFilteringService = mock(SstFilteringService.class);
+ SnapshotDeletingService mockSnapshotDeletingService =
mock(SnapshotDeletingService.class);
+ RocksDBCheckpointDiffer mockCheckpointDiffer =
mock(RocksDBCheckpointDiffer.class);
+ // Create mock locks for each service
+ BootstrapStateHandler.Lock mockDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockDirDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockFilteringLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockSnapshotDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockCheckpointDifferLock =
mock(BootstrapStateHandler.Lock.class);
+ // Configure service mocks to return their respective locks
+
when(mockDeletingService.getBootstrapStateLock()).thenReturn(mockDeletingLock);
+
when(mockDirDeletingService.getBootstrapStateLock()).thenReturn(mockDirDeletingLock);
+
when(mockFilteringService.getBootstrapStateLock()).thenReturn(mockFilteringLock);
+
when(mockSnapshotDeletingService.getBootstrapStateLock()).thenReturn(mockSnapshotDeletingLock);
+
when(mockCheckpointDiffer.getBootstrapStateLock()).thenReturn(mockCheckpointDifferLock);
+ // Mock KeyManager and its services
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.getDeletingService()).thenReturn(mockDeletingService);
+
when(mockKeyManager.getDirDeletingService()).thenReturn(mockDirDeletingService);
+
when(mockKeyManager.getSnapshotSstFilteringService()).thenReturn(mockFilteringService);
+
when(mockKeyManager.getSnapshotDeletingService()).thenReturn(mockSnapshotDeletingService);
+ // Mock OMMetadataManager and Store
+ OMMetadataManager mockMetadataManager = mock(OMMetadataManager.class);
+ DBStore mockStore = mock(DBStore.class);
+ when(mockMetadataManager.getStore()).thenReturn(mockStore);
+
when(mockStore.getRocksDBCheckpointDiffer()).thenReturn(mockCheckpointDiffer);
+ // Mock OzoneManager
+ OzoneManager mockOM = mock(OzoneManager.class);
+ when(mockOM.getKeyManager()).thenReturn(mockKeyManager);
+ when(mockOM.getMetadataManager()).thenReturn(mockMetadataManager);
+ // Create the actual Lock instance (this tests the real implementation)
+ OMDBCheckpointServlet.Lock bootstrapLock = new
OMDBCheckpointServlet.Lock(mockOM);
+ // Test successful lock acquisition
+ BootstrapStateHandler.Lock result = bootstrapLock.lock();
+ // Verify all service locks were acquired
+ verify(mockDeletingLock).lock();
+ verify(mockDirDeletingLock).lock();
+ verify(mockFilteringLock).lock();
+ verify(mockSnapshotDeletingLock).lock();
+ verify(mockCheckpointDifferLock).lock();
+ // Verify double buffer flush was called
+ verify(mockOM).awaitDoubleBufferFlush();
+ // Verify the lock returns itself
+ assertEquals(bootstrapLock, result);
+ // Test unlock
+ bootstrapLock.unlock();
+ // Verify all service locks were released
+ verify(mockDeletingLock).unlock();
+ verify(mockDirDeletingLock).unlock();
+ verify(mockFilteringLock).unlock();
+ verify(mockSnapshotDeletingLock).unlock();
+ verify(mockCheckpointDifferLock).unlock();
+ }
+
+ /**
+ * Verifies that bootstrap lock acquisition blocks background services
during checkpoint creation,
+ * preventing race conditions between checkpoint and service operations.
+ */
+ @Test
+ public void testBootstrapLockBlocksMultipleServices() throws Exception {
+ setupCluster();
+ // Initialize servlet
+ OMDBCheckpointServletInodeBasedXfer servlet = new
OMDBCheckpointServletInodeBasedXfer();
+ ServletConfig servletConfig = mock(ServletConfig.class);
+ ServletContext servletContext = mock(ServletContext.class);
+ when(servletConfig.getServletContext()).thenReturn(servletContext);
+
when(servletContext.getAttribute(OzoneConsts.OM_CONTEXT_ATTRIBUTE)).thenReturn(om);
+ servlet.init(servletConfig);
+
+ BootstrapStateHandler.Lock bootstrapLock = servlet.getBootstrapStateLock();
+ // Test multiple services being blocked
+ CountDownLatch bootstrapAcquired = new CountDownLatch(1);
+ CountDownLatch allServicesCompleted = new CountDownLatch(3); // 3
background services
+ AtomicInteger servicesBlocked = new AtomicInteger(0);
+ AtomicInteger servicesSucceeded = new AtomicInteger(0);
+ // Checkpoint thread holds bootstrap lock
+ Thread checkpointThread = new Thread(() -> {
+ try {
+ LOG.info("Acquiring bootstrap lock for checkpoint...");
+ BootstrapStateHandler.Lock acquired = bootstrapLock.lock();
+ bootstrapAcquired.countDown();
+ Thread.sleep(3000); // Hold for 3 seconds
Review Comment:
The hardcoded 3000ms sleep duration is a magic number. Consider extracting
this to a named constant like CHECKPOINT_LOCK_HOLD_DURATION_MS for better
maintainability.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java:
##########
@@ -438,6 +449,143 @@ public void testWriteDBToArchive(boolean
expectOnlySstFiles) throws Exception {
}
}
+ @Test
+ public void testBootstrapLockCoordination() throws Exception {
+ // Create mocks for all background services
+ KeyDeletingService mockDeletingService = mock(KeyDeletingService.class);
+ DirectoryDeletingService mockDirDeletingService =
mock(DirectoryDeletingService.class);
+ SstFilteringService mockFilteringService = mock(SstFilteringService.class);
+ SnapshotDeletingService mockSnapshotDeletingService =
mock(SnapshotDeletingService.class);
+ RocksDBCheckpointDiffer mockCheckpointDiffer =
mock(RocksDBCheckpointDiffer.class);
+ // Create mock locks for each service
+ BootstrapStateHandler.Lock mockDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockDirDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockFilteringLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockSnapshotDeletingLock =
mock(BootstrapStateHandler.Lock.class);
+ BootstrapStateHandler.Lock mockCheckpointDifferLock =
mock(BootstrapStateHandler.Lock.class);
+ // Configure service mocks to return their respective locks
+
when(mockDeletingService.getBootstrapStateLock()).thenReturn(mockDeletingLock);
+
when(mockDirDeletingService.getBootstrapStateLock()).thenReturn(mockDirDeletingLock);
+
when(mockFilteringService.getBootstrapStateLock()).thenReturn(mockFilteringLock);
+
when(mockSnapshotDeletingService.getBootstrapStateLock()).thenReturn(mockSnapshotDeletingLock);
+
when(mockCheckpointDiffer.getBootstrapStateLock()).thenReturn(mockCheckpointDifferLock);
+ // Mock KeyManager and its services
+ KeyManager mockKeyManager = mock(KeyManager.class);
+ when(mockKeyManager.getDeletingService()).thenReturn(mockDeletingService);
+
when(mockKeyManager.getDirDeletingService()).thenReturn(mockDirDeletingService);
+
when(mockKeyManager.getSnapshotSstFilteringService()).thenReturn(mockFilteringService);
+
when(mockKeyManager.getSnapshotDeletingService()).thenReturn(mockSnapshotDeletingService);
+ // Mock OMMetadataManager and Store
+ OMMetadataManager mockMetadataManager = mock(OMMetadataManager.class);
+ DBStore mockStore = mock(DBStore.class);
+ when(mockMetadataManager.getStore()).thenReturn(mockStore);
+
when(mockStore.getRocksDBCheckpointDiffer()).thenReturn(mockCheckpointDiffer);
+ // Mock OzoneManager
+ OzoneManager mockOM = mock(OzoneManager.class);
+ when(mockOM.getKeyManager()).thenReturn(mockKeyManager);
+ when(mockOM.getMetadataManager()).thenReturn(mockMetadataManager);
+ // Create the actual Lock instance (this tests the real implementation)
+ OMDBCheckpointServlet.Lock bootstrapLock = new
OMDBCheckpointServlet.Lock(mockOM);
+ // Test successful lock acquisition
+ BootstrapStateHandler.Lock result = bootstrapLock.lock();
+ // Verify all service locks were acquired
+ verify(mockDeletingLock).lock();
+ verify(mockDirDeletingLock).lock();
+ verify(mockFilteringLock).lock();
+ verify(mockSnapshotDeletingLock).lock();
+ verify(mockCheckpointDifferLock).lock();
+ // Verify double buffer flush was called
+ verify(mockOM).awaitDoubleBufferFlush();
+ // Verify the lock returns itself
+ assertEquals(bootstrapLock, result);
+ // Test unlock
+ bootstrapLock.unlock();
+ // Verify all service locks were released
+ verify(mockDeletingLock).unlock();
+ verify(mockDirDeletingLock).unlock();
+ verify(mockFilteringLock).unlock();
+ verify(mockSnapshotDeletingLock).unlock();
+ verify(mockCheckpointDifferLock).unlock();
+ }
+
+ /**
+ * Verifies that bootstrap lock acquisition blocks background services
during checkpoint creation,
+ * preventing race conditions between checkpoint and service operations.
+ */
+ @Test
+ public void testBootstrapLockBlocksMultipleServices() throws Exception {
+ setupCluster();
+ // Initialize servlet
+ OMDBCheckpointServletInodeBasedXfer servlet = new
OMDBCheckpointServletInodeBasedXfer();
+ ServletConfig servletConfig = mock(ServletConfig.class);
+ ServletContext servletContext = mock(ServletContext.class);
+ when(servletConfig.getServletContext()).thenReturn(servletContext);
+
when(servletContext.getAttribute(OzoneConsts.OM_CONTEXT_ATTRIBUTE)).thenReturn(om);
+ servlet.init(servletConfig);
+
+ BootstrapStateHandler.Lock bootstrapLock = servlet.getBootstrapStateLock();
+ // Test multiple services being blocked
+ CountDownLatch bootstrapAcquired = new CountDownLatch(1);
+ CountDownLatch allServicesCompleted = new CountDownLatch(3); // 3
background services
+ AtomicInteger servicesBlocked = new AtomicInteger(0);
+ AtomicInteger servicesSucceeded = new AtomicInteger(0);
+ // Checkpoint thread holds bootstrap lock
+ Thread checkpointThread = new Thread(() -> {
+ try {
+ LOG.info("Acquiring bootstrap lock for checkpoint...");
+ BootstrapStateHandler.Lock acquired = bootstrapLock.lock();
+ bootstrapAcquired.countDown();
+ Thread.sleep(3000); // Hold for 3 seconds
+ LOG.info("Releasing bootstrap lock...");
+ acquired.unlock();
+ } catch (Exception e) {
+ fail("Checkpoint failed: " + e.getMessage());
+ }
+ });
+
+ BiFunction<String, BootstrapStateHandler, Thread> createServiceThread =
+ (serviceName, service) -> new Thread(() -> {
+ try {
+ bootstrapAcquired.await();
+ if (service != null) {
+ LOG.info("{} : Trying to acquire lock...", serviceName);
+ servicesBlocked.incrementAndGet();
+ BootstrapStateHandler.Lock serviceLock =
service.getBootstrapStateLock();
+ serviceLock.lock(); // Should block!
+ servicesBlocked.decrementAndGet();
+ servicesSucceeded.incrementAndGet();
+ LOG.info(" {} : Lock acquired!", serviceName);
+ serviceLock.unlock();
+ }
+ allServicesCompleted.countDown();
+ } catch (Exception e) {
+ LOG.error("{} failed", serviceName, e);
+ allServicesCompleted.countDown();
+ }
+ });
+ // Start all threads
+ checkpointThread.start();
+ Thread keyDeletingThread = createServiceThread.apply("KeyDeletingService",
+ om.getKeyManager().getDeletingService());
+ Thread dirDeletingThread =
createServiceThread.apply("DirectoryDeletingService",
+ om.getKeyManager().getDirDeletingService());
+ Thread snapshotDeletingThread =
createServiceThread.apply("SnapshotDeletingService",
+ om.getKeyManager().getSnapshotDeletingService());
+ keyDeletingThread.start();
+ dirDeletingThread.start();
+ snapshotDeletingThread.start();
+ // Wait a bit, then verify multiple services are blocked
+ Thread.sleep(1000);
Review Comment:
The hardcoded 1000ms sleep duration is a magic number. Consider extracting
this to a named constant like SERVICE_BLOCKING_WAIT_MS for better
maintainability.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
