rm shutdown-check as main loop

linkedin · Nov 5, 2023 · ce7cc38 · ce7cc38
1 parent 5c2eb4e
commit ce7cc38
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 4 deletions.
diff --git a/ambry-cloud/src/main/java/com/github/ambry/cloud/CloudStorageCompactor.java b/ambry-cloud/src/main/java/com/github/ambry/cloud/CloudStorageCompactor.java
@@ -18,7 +18,6 @@
 import com.github.ambry.utils.Utils;
 import java.util.HashMap;
 import java.util.Set;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
@@ -63,10 +62,10 @@ public void run() {
     logger.info("[COMPACT] Thread info = {}", Thread.currentThread());
 
     // Main compaction loop
-    while (!isShutDown()) {
+    while (true) {
       logger.info("[COMPACT] Starting cloud compaction");
       compactPartitions();
-      // This check prevents us from falling asleep. The loop condition prevents us from submitting more jobs.
+      // This shutdown-check prevents us from falling asleep.
       if (isShutDown()) {
         logger.info("[COMPACT] Breaking loop because compaction executor is shutdown");
         break;
@@ -148,7 +147,10 @@ public int compactPartitions() {
 
       // Wait for completion or shutdown, don't use any timeouts
       for (String partitionIdStr : compactionTasks.keySet()) {
+        // Although this is a finite loop, the future.get() is a blocking one, and we can't predict how long that'll be.
+        // This shutdown-check short circuits the loop.
         if (executorService.isShutdown()) {
+          logger.info("[COMPACT] Skipping Future.get because of shutdown");
           break;
         }
         try {

diff --git a/ambry-cloud/src/test/java/com/github/ambry/cloud/CloudStorageCompactorTest.java b/ambry-cloud/src/test/java/com/github/ambry/cloud/CloudStorageCompactorTest.java
@@ -27,7 +27,6 @@
 import java.util.concurrent.TimeUnit;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mockito.invocation.InvocationOnMock;
 import org.mockito.junit.MockitoJUnitRunner;
 import org.mockito.stubbing.Answer;
 import org.slf4j.Logger;

diff --git a/ambry-cloud/src/test/java/com/github/ambry/cloud/VcrServerTest.java b/ambry-cloud/src/test/java/com/github/ambry/cloud/VcrServerTest.java
@@ -69,6 +69,7 @@ public void testVCRServerWithStaticCluster() throws Exception {
     VerifiableProperties verifiableProperties = getStaticClusterVcrProps();
     VcrServer vcrServer = new VcrServer(verifiableProperties, mockClusterAgentsFactory, notificationSystem, null);
     vcrServer.startup();
+    Assert.assertNull("Expected null compactor", vcrServer.getVcrReplicationManager().getCloudStorageCompactor());
     vcrServer.shutdown();
   }
 
@@ -116,6 +117,7 @@ public void testVCRServerWithHelixCluster() throws Exception {
         new VcrServer(verifiableProperties, mockClusterAgentsFactory, notificationSystem, cloudDestinationFactory,
             null);
     vcrServer.startup();
+    Assert.assertNotNull("Expected compactor", vcrServer.getVcrReplicationManager().getCloudStorageCompactor());
     vcrServer.shutdown();
     helixControllerManager.syncStop();
     zkInfo.shutdown();