fixes for LookupReferencesManagerTest (#6444)

* some fixes for LookupReferencesManagerTest

* docs

* formatting

* more formatting fixes

docs/content/querying/lookups.md

@@ -361,6 +361,7 @@ It is possible to save the configuration across restarts such that a node will n
 |`druid.lookup.numLookupLoadingThreads`|Number of threads for loading the lookups in parallel on startup. This thread pool is destroyed once startup is done. It is not kept during the lifetime of the JVM|Available Processors / 2|
 |`druid.lookup.coordinatorFetchRetries`|How many times to retry to fetch the lookup bean list from coordinator, during the sync on startup.|3|
 |`druid.lookup.lookupStartRetries`|How many times to retry to start each lookup, either during the sync on startup, or during the runtime.|3|
+|`druid.lookup.coordinatorRetryDelay`|How long to delay (in millis) between retries to fetch lookup list from the coordinator during the sync on startup.|60_000|
 
 ## Introspect a Lookup
 

processing/src/main/java/org/apache/druid/query/lookup/LookupConfig.java

@@ -28,6 +28,7 @@ import java.util.Objects;
 
 public class LookupConfig
 {
+  static int DEFAULT_COORDINATOR_RETRY_DELAY = 60_000;
 
   @JsonProperty("snapshotWorkingDir")
   private String snapshotWorkingDir;
@@ -43,6 +44,13 @@ public class LookupConfig
   @JsonProperty("coordinatorFetchRetries")
   private int coordinatorFetchRetries = 3;
 
+  // By default, add an extra minute in addition to the retry wait. In RetryUtils, retry wait starts from a few
+  // seconds, that is likely not enough to coordinator to be back to healthy state, e. g. if it experiences
+  // 30-second GC pause.
+  @Min(0)
+  @JsonProperty("coordinatorRetryDelay")
+  private int coordinatorRetryDelay = DEFAULT_COORDINATOR_RETRY_DELAY;
+
   @Min(1)
   @JsonProperty("lookupStartRetries")
   private int lookupStartRetries = 3;
@@ -84,6 +92,11 @@ public class LookupConfig
     return lookupStartRetries;
   }
 
+  public int getCoordinatorRetryDelay()
+  {
+    return coordinatorRetryDelay;
+  }
+
   @Override
   public boolean equals(Object o)
   {
@@ -100,7 +113,8 @@ public class LookupConfig
            enableLookupSyncOnStartup == that.enableLookupSyncOnStartup &&
            numLookupLoadingThreads == that.numLookupLoadingThreads &&
            coordinatorFetchRetries == that.coordinatorFetchRetries &&
-           lookupStartRetries == that.lookupStartRetries;
+           lookupStartRetries == that.lookupStartRetries &&
+           coordinatorRetryDelay == that.coordinatorRetryDelay;
   }
 
   @Override
@@ -111,7 +125,8 @@ public class LookupConfig
         enableLookupSyncOnStartup,
         numLookupLoadingThreads,
         coordinatorFetchRetries,
-        lookupStartRetries
+        lookupStartRetries,
+        coordinatorRetryDelay
     );
   }
 
@@ -124,6 +139,7 @@ public class LookupConfig
            ", numLookupLoadingThreads=" + numLookupLoadingThreads +
            ", coordinatorFetchRetries=" + coordinatorFetchRetries +
            ", lookupStartRetries=" + lookupStartRetries +
+           ", coordinatorRetryDelay=" + coordinatorRetryDelay +
            '}';
   }
 }

processing/src/test/java/org/apache/druid/query/lookup/LookupConfigTest.java

@@ -53,7 +53,8 @@ public class LookupConfigTest
                   + "  \"snapshotWorkingDir\": \"/tmp\",\n"
                   + "  \"numLookupLoadingThreads\": 4,\n"
                   + "  \"coordinatorFetchRetries\": 4,\n"
-                  + "  \"lookupStartRetries\": 4 \n"
+                  + "  \"lookupStartRetries\": 4,\n"
+                  + "  \"coordinatorRetryDelay\": 100 \n"
                   + "}\n";
     LookupConfig config = mapper.readValue(
         mapper.writeValueAsString(
@@ -67,5 +68,6 @@ public class LookupConfigTest
     Assert.assertEquals(4, config.getNumLookupLoadingThreads());
     Assert.assertEquals(4, config.getCoordinatorFetchRetries());
     Assert.assertEquals(4, config.getLookupStartRetries());
+    Assert.assertEquals(100, config.getCoordinatorRetryDelay());
   }
 }

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

@@ -389,11 +389,11 @@ public class LookupReferencesManager
           () -> {
             if (firstAttempt.isTrue()) {
               firstAttempt.setValue(false);
-            } else {
-              // Adding an extra minute in addition to the retry wait. In RetryUtils, retry wait starts from a few
-              // seconds, that is likely not enough to coordinator to be back to healthy state, e. g. if it experiences
-              // 30-second GC pause.
-              Thread.sleep(60_000);
+            } else if (lookupConfig.getCoordinatorRetryDelay() > 0) {
+              // Adding any configured extra time in addition to the retry wait. In RetryUtils, retry wait starts from
+              // a few seconds, that is likely not enough to coordinator to be back to healthy state, e. g. if it
+              // experiences 30-second GC pause. Default is 1 minute
+              Thread.sleep(lookupConfig.getCoordinatorRetryDelay());
             }
             return tryGetLookupListFromCoordinator(tier);
           },

server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java

@@ -50,20 +50,15 @@ import static org.easymock.EasyMock.reset;
 
 public class LookupReferencesManagerTest
 {
-  LookupReferencesManager lookupReferencesManager;
-
-  private DruidLeaderClient druidLeaderClient;
-
-  private LookupListeningAnnouncerConfig config;
-
   private static final String LOOKUP_TIER = "lookupTier";
-
-  LookupExtractorFactory lookupExtractorFactory;
-
-  LookupExtractorFactoryContainer container;
   @Rule
   public TemporaryFolder temporaryFolder = new TemporaryFolder();
+  LookupReferencesManager lookupReferencesManager;
+  LookupExtractorFactory lookupExtractorFactory;
+  LookupExtractorFactoryContainer container;
   ObjectMapper mapper = new DefaultObjectMapper();
+  private DruidLeaderClient druidLeaderClient;
+  private LookupListeningAnnouncerConfig config;
 
   @Before
   public void setUp() throws IOException
@@ -85,7 +80,9 @@ public class LookupReferencesManagerTest
     String temporaryPath = temporaryFolder.newFolder().getAbsolutePath();
     lookupReferencesManager = new LookupReferencesManager(
         new LookupConfig(temporaryFolder.newFolder().getAbsolutePath()),
-        mapper, druidLeaderClient, config,
+        mapper,
+        druidLeaderClient,
+        config,
         true
     );
   }
@@ -104,7 +101,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -165,7 +165,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -203,7 +206,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -234,7 +240,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -262,7 +271,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -292,7 +304,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -326,7 +341,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -354,7 +372,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -405,7 +426,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -447,7 +471,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -523,7 +550,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -543,13 +573,31 @@ public class LookupReferencesManagerTest
   @Test
   public void testLoadLookupOnCoordinatorFailure() throws Exception
   {
+    LookupConfig lookupConfig = new LookupConfig(temporaryFolder.newFolder().getAbsolutePath())
+    {
+      @Override
+      public int getCoordinatorRetryDelay()
+      {
+        return 10;
+      }
+    };
+    lookupReferencesManager = new LookupReferencesManager(
+        lookupConfig,
+        mapper,
+        druidLeaderClient,
+        config
+    );
+
     Map<String, Object> lookupMap = new HashMap<>();
     lookupMap.put("testMockForLoadLookupOnCoordinatorFailure", container);
     String strResult = mapper.writeValueAsString(lookupMap);
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request)
         .anyTimes();
     FullResponseHolder responseHolder = new FullResponseHolder(
@@ -564,16 +612,30 @@ public class LookupReferencesManagerTest
     lookupReferencesManager.add("testMockForLoadLookupOnCoordinatorFailure", container);
     lookupReferencesManager.handlePendingNotices();
     lookupReferencesManager.stop();
+    lookupConfig = new LookupConfig(lookupReferencesManager.lookupSnapshotTaker.getPersistFile(LOOKUP_TIER).getParent())
+    {
+      @Override
+      public int getCoordinatorRetryDelay()
+      {
+        return 10;
+      }
+    };
+
     lookupReferencesManager = new LookupReferencesManager(
-        new LookupConfig(lookupReferencesManager.lookupSnapshotTaker.getPersistFile(LOOKUP_TIER).getParent()),
-        mapper, druidLeaderClient, config,
+        lookupConfig,
+        mapper,
+        druidLeaderClient,
+        config,
         true
     );
     reset(config);
     reset(druidLeaderClient);
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request)
         .anyTimes();
     expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes();
@@ -585,9 +647,19 @@ public class LookupReferencesManagerTest
   @Test
   public void testDisableLookupSync() throws Exception
   {
+    LookupConfig lookupConfig = new LookupConfig(null)
+    {
+      @Override
+      public boolean getEnableLookupSyncOnStartup()
+      {
+        return false;
+      }
+    };
     LookupReferencesManager lookupReferencesManager = new LookupReferencesManager(
-        new LookupConfig(null),
-        mapper, druidLeaderClient, config
+        lookupConfig,
+        mapper,
+        druidLeaderClient,
+        config
     );
     Map<String, Object> lookupMap = new HashMap<>();
     lookupMap.put("testMockForDisableLookupSync", container);
@@ -595,7 +667,10 @@ public class LookupReferencesManagerTest
     Request request = new Request(HttpMethod.GET, new URL("http://localhost:1234/xx"));
     expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes();
     replay(config);
-    expect(druidLeaderClient.makeRequest(HttpMethod.GET, "/druid/coordinator/v1/lookups/lookupTier?detailed=true"))
+    expect(druidLeaderClient.makeRequest(
+        HttpMethod.GET,
+        "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true"
+    ))
         .andReturn(request);
     FullResponseHolder responseHolder = new FullResponseHolder(
         HttpResponseStatus.OK,
@@ -607,5 +682,4 @@ public class LookupReferencesManagerTest
     lookupReferencesManager.start();
     Assert.assertNull(lookupReferencesManager.get("testMockForDisableLookupSync"));
   }
-
 }