[QTL][Lookup] lookup module with the snapshot utility #2517

b-slim · 2016-02-22T21:23:07Z

This PR

Refactor lookup class into one package and one module.
Add the ability of bootstraping lookup configuration from a snapshot file. The snapshot file will be created and periodically updated by the lookupFactoryRefManager. This will ensure that we dont supply configuration via runtime property file.
@drcrallen config and module classes in this PR need to be merged with [QTL] Query time lookup cluster wide config #1576

cheddar · 2016-02-23T17:09:11Z

docs/content/querying/lookups.md

+
+|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|


I wonder if this should be required instead of optional.

thought about it but felt like ppls might chose to not use it. i can set it as mandatory if you feel that we can not use Lookup without this snapshot utility , which in MO not true.

It's possible to use segments without the local segment cache as well, but we force that because it allows the node to be restarted in the exact same state it was in before it went down. This should be the same.

Actually, though, I think it would make sense to maybe make it default to the segmentCache directory + "/lookups" or something like that.

@cheddar but thought broker doesn't have segment cache property ?

Good call. Yes, it does not...

b-slim · 2016-02-24T14:31:55Z

@drcrallen can you please take a look at this.

cheddar · 2016-02-24T18:34:41Z

processing/src/main/java/io/druid/query/lookup/LookupReferencesManager.java

+
+      boolean isAdded = (null == lookupMap.putIfAbsent(lookupName, lookupExtractorFactory));
+      if (isAdded && lookupSnapshotTaker != null) {
+        lookupSnapshotTaker.takeSnapshot(getAllAsList());


This works. But, why re-write all of them instead of just appending the new one?

we don't expect this will change very frequently and size will be limited so i think let's keep it this way

Does this need to have a snapshot every put instead of just on stop()?

yes every put you have new changes, like that you make sure if the process crash we have a recent snapshot.

cheddar · 2016-02-24T18:39:24Z

I left some comments that I could be taken or could be not taken. I think it's fine as is and am ready to 👍 as is if none of the feedback is taken.

drcrallen · 2016-02-24T19:43:27Z

FYI there is an issue here hinted at in #1576

If a node is down when a DROP request propagates across the cluster, the coordinator has no idea if the Lookups a node contains when it comes back up are custom added or still present from the failed DROP (since the node was down)

b-slim · 2016-02-29T15:56:28Z

@drcrallen can you please take look at this one it is sort of easy to pull in.

drcrallen · 2016-03-08T21:29:23Z

extensions/namespace-lookup/src/main/java/io/druid/query/extraction/NamespacedExtractor.java

@@ -27,6 +27,7 @@
 import com.google.common.base.Preconditions;
 import com.google.inject.name.Named;
 import com.metamx.common.StringUtils;
+import io.druid.query.lookup.LookupExtractor;


oh nvmnd, package refactor

drcrallen · 2016-03-08T21:59:23Z

processing/src/test/java/io/druid/query/lookup/LookupSnapshotTakerTest.java

+  @After
+  public void tearDown()
+  {
+    temporaryFolder.delete();


drcrallen · 2016-03-08T22:04:08Z

General comments:

Approach is fine IF this is a path that is desired to go. An alternative approach would be to have a static config and have the node read from that static config independent of the central config management (which is essentially what this PR does)
[QTL][Lookup] lookup module with the snapshot utility #2517 (comment) is still not adressed
Package refactor is good
Needs more clarification and tests around handling error cases (file corruption or failure to write to disk).
Suggest moving from List<LookupBean> to Map<String, LookupExtractorFactory>

b-slim · 2016-03-08T22:55:24Z

Approach is fine IF this is a path that is desired to go. An alternative approach would be to have a static config and have the node read from that static config independent of the central config management (which is essentially what this PR does)

Talked to @cheddar about that and we think that the best way to go is to avoid the static path since it will introduce the case where after restart the old config is out of synch. But as you said it is basically the same thing.

b-slim · 2016-03-08T22:58:15Z

#2517 (comment) is still not adressed

This will be manage by PR #1576. In my opinion coordinator will have a list of to drop and try to re send the drop request if the current diff show it is still there. That's why i think we should not compare to prior but the the actual view.

b-slim · 2016-03-08T22:58:43Z

Needs more clarification and tests around handling error cases (file corruption or failure to write to disk).

Sure Will do

b-slim · 2016-03-09T14:21:26Z

@drcrallen thanks for the review, i have addressed all the comments, please take a look.

drcrallen · 2016-03-09T23:10:32Z

processing/src/main/java/io/druid/query/lookup/LookupReferencesManager.java

+      LOGGER.debug("Removed lookup [%s]", lookupName);
+      if (!isClosed() && lookupSnapshotTaker != null) {
+        // if it is closed the final snapshot will be taken at stop() method
+        lookupSnapshotTaker.takeSnapshot(getAllAsList());


Sometimes this is protected by the start/stop lock and sometimes it is not. It seems that it should probably be protected here. It is possible to pass isClosed, but have stop remove elements between isClosed and getAllAsList

i am not sure i get this. For all the put and remove i am asserting that is started and the block is synchronized around lock.

synchronized (lock) { assertStarted();

put has the code you mention, remove does not

b-slim · 2016-03-11T16:17:41Z

@drcrallen anything is blocking this ?

drcrallen · 2016-03-11T19:51:57Z

@b-slim #2517 (comment)

b-slim · 2016-03-11T19:59:38Z

@drcrallen thanks i have added lock

b-slim · 2016-03-14T14:55:33Z

@drcrallen This is squashed and addressed all the comments, can we get this in, it is blocking me please.

drcrallen · 2016-03-14T21:08:45Z

processing/src/main/java/io/druid/query/lookup/LookupSnapshotTaker.java

+public class LookupSnapshotTaker
+{
+  private static final Logger LOGGER = new Logger(LookupSnapshotTaker.class);
+  private static final String PERSIST_FILE_NAME = "lookupSnapshot.data";


suggest .json

drcrallen · 2016-03-14T21:45:53Z

Minor comments, but for #2517 (comment) can the module be moved into the list of modules per CLIXXX ?

b-slim · 2016-03-15T14:42:32Z

@drcrallen done.

drcrallen · 2016-03-15T16:22:17Z

processing/src/main/java/io/druid/query/lookup/LookupReferencesManager.java

+        if (lookupSnapshotTaker != null) {
+          final List<LookupBean> lookupBeanList = lookupSnapshotTaker.pullExistingSnapshot();
+          for (LookupBean lookupBean : lookupBeanList
+              ) {


formatting looks weird here

drcrallen · 2016-03-15T16:25:26Z

Overall code is in a very good spot. Minor syntactical comments that I'll leave to the author's discretion, but please squash, and I'm 👍 after squash and travis passes.

drcrallen · 2016-03-15T16:26:25Z

I'll reconcile this with #2517 as soon as it is merged.

[QTL][Lookup] lookup module with the snapshot utility

drcrallen · 2016-03-17T18:07:16Z

Reconciled. I had to move the Module into druid-server

cheddar reviewed Feb 23, 2016
View reviewed changes

b-slim added this to the 0.9.1 milestone Feb 24, 2016

b-slim added the Feature label Feb 24, 2016

b-slim assigned drcrallen Feb 24, 2016

b-slim changed the title ~~lookup module with the snapshot utility~~ [QTL][Lookup] lookup module with the snapshot utility Feb 24, 2016

b-slim mentioned this pull request Feb 24, 2016

Support lookups at Query Time #2329

Closed

cheddar reviewed Feb 24, 2016
View reviewed changes

b-slim force-pushed the adding_lookup_snapshot_utility branch from ef75638 to 177d6f1 Compare February 24, 2016 20:33

drcrallen reviewed Mar 8, 2016
View reviewed changes

b-slim force-pushed the adding_lookup_snapshot_utility branch from 177d6f1 to 0239bed Compare March 9, 2016 14:16

drcrallen reviewed Mar 9, 2016
View reviewed changes

b-slim force-pushed the adding_lookup_snapshot_utility branch from 0239bed to 877096f Compare March 11, 2016 16:20

b-slim force-pushed the adding_lookup_snapshot_utility branch from 8e01b45 to 4915f4c Compare March 14, 2016 14:54

drcrallen reviewed Mar 14, 2016
View reviewed changes

drcrallen reviewed Mar 15, 2016
View reviewed changes

lookup module with the snapshot utility

0c86b29

b-slim force-pushed the adding_lookup_snapshot_utility branch from d7466fc to 0c86b29 Compare March 17, 2016 14:22

b-slim added a commit that referenced this pull request Mar 17, 2016

Merge pull request #2517 from b-slim/adding_lookup_snapshot_utility

cf342d8

[QTL][Lookup] lookup module with the snapshot utility

b-slim merged commit cf342d8 into apache:master Mar 17, 2016

clambertus unassigned drcrallen Jul 6, 2018

[QTL][Lookup] lookup module with the snapshot utility #2517

[QTL][Lookup] lookup module with the snapshot utility #2517

Conversation

b-slim commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-slim commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheddar commented Feb 24, 2016

drcrallen commented Feb 24, 2016

b-slim commented Feb 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Mar 8, 2016

b-slim commented Mar 8, 2016

b-slim commented Mar 8, 2016

b-slim commented Mar 8, 2016

b-slim commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-slim commented Mar 11, 2016

drcrallen commented Mar 11, 2016

b-slim commented Mar 11, 2016

b-slim commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Mar 14, 2016

b-slim commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drcrallen commented Mar 15, 2016

drcrallen commented Mar 15, 2016

drcrallen commented Mar 17, 2016