[QTL] Implement LookupExtractorFactory of namespaced lookup by drcrallen · Pull Request #2926 · apache/druid

drcrallen · 2016-05-05T20:35:07Z

Migration of #2716 to a repository more people have commit access to.

This patch adds LookupExtractorFactory of namespaced lookup.
With this, NamespacedExtractor can be used with LookupDimensionSpec.
Done as a first step to merge #2524 with QTL.

… eliminate static configurations for lookup from namespecd lookup extensions - druid-namespace-lookup and druid-kafka-extraction-namespace are modified - However, druid-namespace-lookup still has configuration about ON/OFF HEAP cache manager selection, which is not namespace wide configuration but node wide configuration as multiple namespace shares the same cache manager

…nges

…tory

…amx/druid into metamx-namespaceLookupMovetoLookups

…_lookup

Merge master lookups

b-slim · 2016-05-05T20:48:49Z

+    try {
+      if (!started.compareAndSet(false, true)) {
+        LOG.warn("Already started!");
+        return false;


return true ?

fixed both cases and tests

b-slim · 2016-05-06T14:34:24Z

+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+@JsonTypeName("namespace")


i think we need to call this something that reflects that is is a global cached lookup manager. How about CachedNamespace ?

sounds good will change

b-slim · 2016-05-06T14:51:25Z

+          manager.delete(extractorID);
+          throw new ISE("Lookup [%s] is deleting", extractorID);
+        }
+      } while (!preVersion.equals(postVersion));


i am totally lost, why the version will change between two calls in the same function ?

race condition during background update. If the updating task swaps the cache at a very inopportune time.

but isn't when you get the read lock this can not happen ?

The read lock is only for the entire service shutting down, not for individual namespaces

fjy · 2016-05-20T22:56:58Z

👍 but I didn't read the tests very carefully

xvrl · 2016-05-20T23:03:59Z

+  public boolean start()
+  {
+    final Lock writeLock = startStopSync.writeLock();
+    writeLock.lock();


is there really a benefit to using a read-write lock here? was a simple synchronized block not fast enough?

I didn't want io.druid.query.lookup.NamespaceLookupExtractorFactory#get to get caught up for multiple things calling get

I did not benchmark vs synchronize

xvrl · 2016-05-20T23:45:50Z

+  }
+
+  @JsonIgnore
+  private final AtomicBoolean started = new AtomicBoolean(false);


a simple volatile should be enough here, given that writes are always synchronized on the lock

I like AtomicBoolean and the explicit CAS it has, but it doesn't add much here, so I went ahead and moved to volatile.

b-slim · 2016-05-22T21:46:58Z

@drcrallen can we name the artifact id to something like druid-globalCached-lookups ? like that the second module will be called druid-singleCached-lookups ?

…_no_kafka

drcrallen · 2016-05-23T15:41:52Z

@b-slim renamed to lookups-cached-global

b-slim · 2016-05-23T18:47:50Z

I am not sure i can see the change or the comment it self

On May 23, 2016, at 10:42 AM, Charles Allen notifications@github.com wrote:

@b-slim https://github.com/b-slim renamed to lookups-cached-global ((my immediate prior comment seems to have vanished)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #2926 (comment)

fjy · 2016-05-23T20:18:57Z

@b-slim I see the change. Hard refresh the webpage?

b-slim · 2016-05-23T20:31:35Z

Yes i do now.

On May 23, 2016, at 3:19 PM, Fangjin Yang notifications@github.com wrote:

@b-slim https://github.com/b-slim I see the change. Hard refresh the webpage?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #2926 (comment)

b-slim · 2016-05-23T20:35:20Z


 ## Configuration
+<div class="note caution">
+Static configuration is no longer supported. Only cluster wide configuration is supported


@fjy is there a shard file or memo where developers can list what is backward incompatible or removed functionalities like that you don't have to figure out that your self ? any way this need to be announced as BIC.

If this is too wild of a change, I can copy the old extension over and deprecate it.

(probably separate from this PR)

I dont think we need to do that as QTL as been labeled experimental and we clearly state we can and will change the API at any time.

Cool sounds good

b-slim · 2016-05-23T20:48:36Z

minor comments but overall 👍

* Also add serde test

sirpkt and others added 14 commits March 31, 2016 10:13

update KafkaExtractionNamespaceTest to reflect argument signature cha…

7d75062

…nges

Add more synchronization functionality to NamespaceLookupExtractorFac…

e4ae726

…tory

Remove old way of using extraction namespaces

a525a5f

Merge remote-tracking branch 'upstream/master' into qtl_namespace_lookup

153925a

Merge branch 'namespaceLookupMovetoLookups' of https://github.com/met…

c5cc36a

…amx/druid into metamx-namespaceLookupMovetoLookups

Merge branch 'metamx-namespaceLookupMovetoLookups' into qtl_namespace…

3b45ae2

…_lookup

Merge remote-tracking branch 'upstream/master' into qtl_namespace_lookup

379af21

resolve compile error by supporting LookupIntrospectHandler

5b35e42

Merge remote-tracking branch 'druid/master' into mergeMasterLookups

688e7c1

Merge pull request #2 from metamx/mergeMasterLookups

10fc4f8

Merge master lookups

Merge remote-tracking branch 'upstream/master' into qtl_namespace_lookup

f2b6864

Remove kafka lookups

00f42c1

Remove unused stuff

4e91b13

drcrallen added the Improvement label May 5, 2016

drcrallen added this to the 0.9.1 milestone May 5, 2016

sirpkt mentioned this pull request May 5, 2016

[QTL] Implement LookupExtractorFactory of namespaced lookup #2716

Closed

drcrallen mentioned this pull request May 5, 2016

[QTL] Immediate future plans #2889

Closed

b-slim reviewed May 5, 2016
View reviewed changes

Fix start and stop behavior to be consistent with new javadocs

c23e06d

b-slim reviewed May 6, 2016
View reviewed changes

Remove unused strings

1b3e6cc

b-slim reviewed May 6, 2016
View reviewed changes

drcrallen added 2 commits May 6, 2016 08:11

Add timeout option

8a77bc7

Address comments on configurations and improve docs

d780468

xvrl reviewed May 20, 2016
View reviewed changes

drcrallen added 3 commits May 20, 2016 16:29

Sanity renames and fix imports

6283dc2

Remove extra info in docs

c9db080

Fix review comments

fa1c0c1

xvrl reviewed May 20, 2016
View reviewed changes

drcrallen added 2 commits May 20, 2016 16:47

Strengthen failure on start from warn to error

38ca68e

Address comments

6762c91

drcrallen added 2 commits May 23, 2016 07:48

Merge remote-tracking branch 'druid/master' into qtl_namespace_lookup…

2330549

…_no_kafka

Rename namespace-lookup to lookups-cached-global

15363e0

b-slim reviewed May 23, 2016
View reviewed changes

Fix injective mis-naming

9900d99

* Also add serde test

fjy closed this May 24, 2016

fjy reopened this May 24, 2016

fjy added the Incompatible label May 24, 2016

drcrallen mentioned this pull request May 24, 2016

[WIP] Druid 0.9.1 Release Notes #2999

Closed

fjy merged commit 8024b91 into apache:master May 24, 2016

drcrallen deleted the qtl_namespace_lookup_no_kafka branch May 24, 2016 18:45

jihoonson mentioned this pull request Feb 13, 2021

Remove stale 'namespace' config for JDBC lookups from doc #10886

Merged

9 tasks

Conversation

drcrallen commented May 5, 2016

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!

fjy commented May 20, 2016

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!

b-slim commented May 22, 2016

Uh oh!

drcrallen commented May 23, 2016

Uh oh!

b-slim commented May 23, 2016

Uh oh!

fjy commented May 23, 2016

Uh oh!

b-slim commented May 23, 2016

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!

b-slim commented May 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants