Synchronization of lookups during startup of druid processes by a2l007 · Pull Request #4758 · apache/druid

a2l007 · 2017-09-06T20:45:06Z

This introduces lookup synchronization during startup of Druid processes. The main motivation for this is to cut the lookup load time when performed by the Coordinator, so that realtime tasks have lookups available immediately.
Upon startup, processes communicate with the coordinator on retrieving the lookups to be loaded and loads them before finishing up the start sequence. Thus all the processes have the lookups loaded before starting to serve queries. If it fails to communicate with the coordinator (Coordinator unavailable) then it falls back to snapshot directory if there is any.
This does not fail startup of the process in case of failing to load lookups. Following are the high level changes:

Add synchronization of lookups in LookupReferencesManager
Parallelize the lookup loading process in LookupReferencesManager
Move LookupReferencesManager and associated classes to druid-server module. This was necessary as LookupReferencesManager required access to LookupListeningAnnouncerConfig for tier related information and this was not possible from the druid-processing module.
Introduce a flag which can be used to disable the lookup synchronization workflow on startup.

himanshug · 2017-09-06T20:56:09Z

  private final String snapshotWorkingDir;

+  @JsonProperty
+  private int numLookupLoadingThreads = 10;


can you update the documentation with this and also mention that this threadpool is only created at the time of start and destroyed , it is not kept during the lifetime of jvm.

himanshug · 2017-09-06T20:56:26Z

+  private int numLookupLoadingThreads = 10;
+
+  @JsonProperty
+  private boolean disableLookupSync = false;


disableLookupSyncOnStartup ?

beyond the startup, they are always synced.

himanshug · 2017-09-06T20:58:09Z

+                    LockSupport.parkNanos(LookupReferencesManager.this, TimeUnit.MINUTES.toNanos(1));
+                  }
+                  catch (Throwable t) {
+                    LOG.makeAlert(t, "Error occured while lookup notice handling.").emit();


is this just indentation change? ... why did it change ?

The indentation changed when I changed the Runnable right above it.

himanshug · 2017-09-06T20:59:48Z

+      String tier = lookupListeningAnnouncerConfig.getLookupTier();
+      List<LookupBean> lookupBeanList = new ArrayList<>();
+      // Check if the coordinator is accessible
+      if (getCoordinatorUrl().isEmpty()) {


can you put this check inside getLookupListFromCoordinator(tier) instead ?

himanshug · 2017-09-06T21:05:46Z

+        ListeningScheduledExecutorService executorService = MoreExecutors.listeningDecorator(
+            Executors.newScheduledThreadPool(
+                lookupConfig.getNumLookupLoadingThreads(),
+                Execs.makeThreadFactory("LookupReferencesManager--%s")


can we name it different from the other long running .e.g LookupReferencesManager-startup-%s ?

himanshug · 2017-09-06T21:06:58Z

+
+                      LookupExtractorFactoryContainer container = lookupBean.getContainer();
+                      if (container.getLookupExtractorFactory().start()) {
+                        LOG.info(


can we add this log before calling start() ... "Starting lookup..." ? basically if some start() implementation is stuck for some reason then logs would tell which lookup is stuck on start().

himanshug · 2017-09-06T21:10:11Z

+        stateRef.set(new LookupUpdateState(ImmutableMap.of(), ImmutableList.of(), ImmutableList.of()));
+      }
+    } else {
+      LOG.info("Lookup synchronization is disabled");


can you change it to "Lookup loading on startup is disabled" .

himanshug · 2017-09-06T21:10:56Z

+            response.getStatus(),
+            response.getContent()
+        );
+        if (lookupSnapshotTaker != null) {


this check does not belong here, this should just return null.

himanshug · 2017-09-06T21:11:10Z

+            response.getContent()
+        );
+        if (lookupSnapshotTaker != null) {
+          LOG.info("Attempting to load saved snapshot");


himanshug · 2017-09-06T21:12:44Z

+      } else {
+        // Older version of getSpecificTier returns a list of lookup names.
+        // Lookup loading is performed via snapshot if older version is present.
+        if (response.getContent().startsWith("[")) {


can you update comment to say, this check should be removed in a future release and is there only for backward compatibility?

himanshug · 2017-09-06T21:13:39Z

-      stateRef.set(new LookupUpdateState(ImmutableMap.of(), ImmutableList.of(), ImmutableList.of()));
+  private List<LookupBean> getLookupListFromSnapshot()
+  {
+    return lookupSnapshotTaker.pullExistingSnapshot();


we should do the null check for lookupSnapshotTaker here

himanshug · 2017-09-06T21:14:21Z

+    catch (Exception e) {
+      LOG.error("Error encountered while connecting to Coordinator");
+    }
+    return "";


return null ?

and also this should probably be inside the catch block

himanshug · 2017-09-06T21:14:47Z

+      final Server instance = selector.pick();
+      if (instance == null) {
+        LOG.info("Coordinator instance unavailable.");
+        return "";


return null ?

himanshug · 2017-09-06T21:16:26Z

+    ).get();
+  }
+
+  private LookupExtractorFactoryContainer getLookupEntryFromCode(String tier, String lookupCode)


this is not needed probably.

drcrallen · 2017-09-06T21:47:38Z

    @JsonSubTypes.Type(name = "timeFormat", value = TimeFormatExtractionFn.class),
    @JsonSubTypes.Type(name = "identity", value = IdentityExtractionFn.class),
    @JsonSubTypes.Type(name = "lookup", value = LookupExtractionFn.class),
-    @JsonSubTypes.Type(name = "registeredLookup", value = RegisteredLookupExtractionFn.class),


why is this removed?

I had to move this to https://github.com/a2l007/druid/blob/8f2a360119be5d619a2bc0d57976f483faaad610/server/src/main/java/io/druid/query/lookup/LookupModule.java#L89

as part of the refactoring.

drcrallen · 2017-09-06T21:47:46Z

    @JsonSubTypes.Type(name = "extraction", value = ExtractionDimensionSpec.class),
    @JsonSubTypes.Type(name = "regexFiltered", value = RegexFilteredDimensionSpec.class),
-    @JsonSubTypes.Type(name = "listFiltered", value = ListFilteredDimensionSpec.class),
-    @JsonSubTypes.Type(name = "lookup", value = LookupDimensionSpec.class)


why is this removed?

I had to move this to https://github.com/a2l007/druid/blob/8f2a360119be5d619a2bc0d57976f483faaad610/server/src/main/java/io/druid/query/lookup/LookupModule.java#L89

as part of the refactoring.

a2l007 · 2017-09-07T02:00:04Z

@himanshug Suggested changes have been made. Please review.

…ookups

a2l007 · 2017-09-13T16:18:44Z

Requests to coordinator are now done via DruidLeaderClient.

himanshug · 2017-09-13T19:26:36Z

 |Property|Description|Default|
 |--------|-----------|-------|
 |`druid.lookup.snapshotWorkingDir`| Working path used to store snapshot of current lookup configuration, leaving this property null will disable snapshot/bootstrap utility|null|
+|`druid.lookup.numLookupLoadingThreads`| Number of threads for loading the lookups in parallel.This threadpool is created only at the time of start and destroyed once startup is done. It is not kept during the lifetime of the JVM.|10|


nit: "..lookups in parallel on startup. .."

himanshug · 2017-09-13T19:27:23Z

+  private int numLookupLoadingThreads = 10;
+
+  @JsonProperty
+  private boolean disableLookupSyncOnStartup = false;


can you document this as well ?

himanshug · 2017-09-13T19:31:35Z

+
+                      LookupExtractorFactoryContainer container = lookupBean.getContainer();
+                      LOG.info(
+                          "Started lookup [%s]:[%s]",


s/Started/Starting

himanshug · 2017-09-13T19:32:04Z

+                          container
+                      );
+                      if (container.getLookupExtractorFactory().start()) {
+                        return new AbstractMap.SimpleImmutableEntry<>(lookupBean.getName(), container);


add a log here with "Started lookup..."

himanshug · 2017-09-13T19:33:24Z

+          }
+          stateRef.set(new LookupUpdateState(builder.build(), ImmutableList.of(), ImmutableList.of()));
+        }
+        catch (Exception e) {


we should set thread to interrupted state if it was an interrupted exception

himanshug · 2017-09-13T19:35:30Z

+          LOG.error(e, "Failed to finish lookup load process.");
+        }
+        executorService.shutdownNow();
+      } else {


can you also wait for executorService to finish using executorService.awaitTermination(1 minute) ... if it doesn't finish within that duration then log a warning.

leventov · 2017-10-03T19:40:51Z

+  {
+    ImmutableMap.Builder<String, LookupExtractorFactoryContainer> builder = ImmutableMap.builder();
+    ListeningScheduledExecutorService executorService = MoreExecutors.listeningDecorator(
+        Executors.newScheduledThreadPool(


Could use Execs.multiThreaded()

leventov · 2017-10-03T19:48:05Z

+  private void startLookups(List<LookupBean> lookupBeanList)
+  {
+    ImmutableMap.Builder<String, LookupExtractorFactoryContainer> builder = ImmutableMap.builder();
+    ListeningScheduledExecutorService executorService = MoreExecutors.listeningDecorator(


Could you please rewrite it using standard Java API, ExecutorCompletionService?

I see that ScheduledExecutorService is not required here. I have changed it to ListeningExecutorService. Is this sufficient or do I still need to refactor it to use ExecutorCompletionService?

I suggest to use ExecutorCompletionService because it's standard Java API, which replaces Guava functionality that you used, that means that this functionality has a good chance of being deprecated and removed from Guava in the future, adding compatibility problems.

Because of this dumb Guava's compatibility policy (remove things), we better use Guava as little as possible.

…ookups

Squashed commit of the following: commit 24eb9fb Merge: eae35a5 07aa405 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Tue Oct 3 21:40:03 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit eae35a5 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Tue Oct 3 21:39:59 2017 -0500 Update LRM commit 37ae0c9 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Tue Oct 3 11:46:38 2017 -0500 Make executorservice local commit fc9d53e Merge: bff5b09 c19cd23 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Tue Oct 3 11:45:31 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit bff5b09 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Mon Oct 2 21:37:10 2017 -0500 Add tests to LookupConfig commit 1cb6627 Merge: 2ea72af 6f91d9c Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Mon Oct 2 19:45:07 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 2ea72af Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Mon Oct 2 08:57:58 2017 -0500 Revert Lookupconfig constructor changes commit c7ecfdc Merge: 9876ae0 1f2074c Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 28 17:14:08 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 9876ae0 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 28 17:14:02 2017 -0500 Remove lookup config constructor commit 99c6f60 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 28 11:50:53 2017 -0500 Refactoring and doc changes commit ce32e04 Merge: 361d255 2c30d5b Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 28 08:14:39 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 361d255 Merge: e5fe3c2 c3fbe51 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 28 08:14:35 2017 -0500 Doc changes commit e5fe3c2 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 20 21:36:16 2017 -0500 Update LookupConfig commit 6d9278f Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 20 21:33:31 2017 -0500 Move executorservice shutdown to finally block commit 263dfdc Merge: 727a706 a36adc6 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 20 20:54:04 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 727a706 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Fri Sep 15 08:59:12 2017 -0500 Rename flag commit 1db8914 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Fri Sep 15 08:37:33 2017 -0500 Update docs commit 756098b Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 14 22:45:57 2017 -0500 Make disablelookups flag true by default commit e9b3ddc Merge: f3ad262 d37be5e Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Thu Sep 14 21:43:28 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit f3ad262 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 13 16:01:00 2017 -0500 Wait before thread shutdown commit 4a11a49 Merge: 6731438 4f6eb47 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 13 15:32:19 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 6731438 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 13 11:15:01 2017 -0500 Change coordinator instance to be retrieved by DruidLeaderClient commit a3bfc31 Merge: 6eaffd4 587f180 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 13 10:52:39 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 6eaffd4 Merge: 317e4e3 3a29521 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Tue Sep 12 16:13:32 2017 -0500 Merge branch 'master' of https://github.com/druid-io/druid into synclookups commit 317e4e3 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 6 20:56:53 2017 -0500 Minor refactors and doc update commit 8f2a360 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 6 15:58:35 2017 -0500 Refactor of Lookup classes commit 5d41b29 Author: Atul Mohan <atulmohan@yahoo-inc.com> Date: Wed Sep 6 15:12:08 2017 -0500 Changes for lookup synchronization

…ookups

a2l007 · 2017-10-04T20:32:18Z

+        "LookupReferencesManager-Startup-%s"
+    );
+    ExecutorCompletionService completionService = new ExecutorCompletionService(executorService);
+    try {


@leventov ExecutorCompletionService is being used in the latest commit. Please review.

leventov · 2017-10-04T20:43:39Z

+                     builder.put(lookupResult);
+                   }
+                 }
+                 catch (InterruptedException | ExecutionException e) {


On InterruptedException, it should fall through to the catch (Exception e) { block below. And cancel all the remaining futures.

I have cancelled the remaining futures on ExecutionException.

leventov · 2017-10-04T20:44:20Z

+            }
+        );
      }
+      IntStream.range(0, lookupBeanList.size())


Please just use loop, also it will make easier to fall thorough with InterruptedException below.

leventov · 2017-10-04T21:28:30Z

    catch (Exception e) {
      LOG.error(e, "Failed to finish lookup load process.");
+      for (Future future : futures) {
+        if (!future.isDone()) {


isDone not needed -- see example in ExecutorCompletionService's javadoc.

leventov · 2017-10-06T22:31:43Z

-        }
+  private List<LookupBean> getLookupsListFromLookupConfig()
+  {
+    List<LookupBean> lookupBeanList = new ArrayList<>();


Pointless assignment

leventov · 2017-10-06T22:32:48Z

+      // Lookup loading is performed via snapshot if older version is present.
+      // This check is only for backward compatibility and should be removed in a future release
+      if (response.getContent().startsWith("[")) {
+        return null;


Please add logging message here, this is a very confusing situation in practice, is just says "coordinator not available"

…ookups

leventov · 2017-10-12T17:33:42Z

@a2l007 could you please finish this?

a2l007 · 2017-10-12T17:55:24Z

@leventov I have updated the logging as requested. Do you have any pending feedback?

leventov · 2017-10-12T21:43:38Z

@himanshug @drcrallen more comments?

himanshug · 2017-10-13T02:22:18Z

LGTM

…mit was one in the series of attempts to fix the problem with QTL startup, that was ultimately fixed by apache#4758. So this commit is probably not needed anymore, but I’m not 100% sure. In our Druid 0.11.0 deployments, this commit was definitely harmless, so not risking and moving this commit to `0.12.1-mmx` for the sake of stability.

Changes for lookup synchronization

5d41b29

himanshug reviewed Sep 6, 2017

View reviewed changes

Refactor of Lookup classes

8f2a360

himanshug reviewed Sep 6, 2017

View reviewed changes

drcrallen reviewed Sep 6, 2017

View reviewed changes

Minor refactors and doc update

317e4e3

Atul Mohan added 3 commits September 12, 2017 16:13

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

6eaffd4

…ookups

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

a3bfc31

…ookups

Change coordinator instance to be retrieved by DruidLeaderClient

6731438

himanshug reviewed Sep 13, 2017

View reviewed changes

leventov requested changes Oct 3, 2017

View reviewed changes

Atul Mohan added 2 commits October 3, 2017 21:39

Update LRM

eae35a5

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

24eb9fb

…ookups

Atul Mohan added 2 commits October 4, 2017 15:26

Move ListeningScheduledExecutorService to ExecutorCompletionService

f80fbd9

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

7e788fa

…ookups

a2l007 commented Oct 4, 2017

View reviewed changes

leventov requested changes Oct 4, 2017

View reviewed changes

Move exception to outer block

8aa5d4f

leventov reviewed Oct 4, 2017

View reviewed changes

Remove check to see future is done

840c644

leventov requested changes Oct 6, 2017

View reviewed changes

Atul Mohan added 3 commits October 6, 2017 21:55

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

f9465d1

…ookups

Remove unnecessary assignment

ef3ead5

Merge branch 'master' of https://github.com/druid-io/druid into syncl…

2bf45e1

…ookups

Add logging

2529959

leventov approved these changes Oct 12, 2017

View reviewed changes

himanshug merged commit c07678b into apache:master Oct 13, 2017

a2l007 deleted the synclookups branch October 16, 2017 13:53

a2l007 restored the synclookups branch October 17, 2017 17:25

a2l007 mentioned this pull request Oct 17, 2017

Backport of lookup sync on startup to 0.11.0 #4969

Merged

gianm mentioned this pull request Nov 2, 2017

Basic auth extension #4823

Closed

jon-wei added this to the 0.11.0 milestone Nov 16, 2017

leventov added the Area - Lookups label Apr 26, 2018

leventov mentioned this pull request Apr 26, 2018

Race Condition on Lookup Status Endpoint #5707

Closed

Conversation

a2l007 commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a2l007 commented Sep 7, 2017

Uh oh!

a2l007 commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

himanshug Sep 6, 2017 •

edited

Loading