-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[QTL] Implement LookupExtractorFactory of namespaced lookup #2926
Conversation
… 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
…amx/druid into metamx-namespaceLookupMovetoLookups
Merge master lookups
try { | ||
if (!started.compareAndSet(false, true)) { | ||
LOG.warn("Already started!"); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return true ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed both cases and tests
import java.util.concurrent.locks.ReadWriteLock; | ||
import java.util.concurrent.locks.ReentrantReadWriteLock; | ||
|
||
@JsonTypeName("namespace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to call this something that reflects that is is a global cached lookup manager. How about CachedNamespace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good will change
manager.delete(extractorID); | ||
throw new ISE("Lookup [%s] is deleting", extractorID); | ||
} | ||
} while (!preVersion.equals(postVersion)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am totally lost, why the version will change between two calls in the same function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
race condition during background update. If the updating task swaps the cache at a very inopportune time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but isn't when you get the read lock this can not happen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read lock is only for the entire service shutting down, not for individual namespaces
👍 but I didn't read the tests very carefully |
public boolean start() | ||
{ | ||
final Lock writeLock = startStopSync.writeLock(); | ||
writeLock.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there really a benefit to using a read-write lock here? was a simple synchronized block not fast enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want io.druid.query.lookup.NamespaceLookupExtractorFactory#get
to get caught up for multiple things calling get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not benchmark vs synchronize
} | ||
|
||
@JsonIgnore | ||
private final AtomicBoolean started = new AtomicBoolean(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a simple volatile should be enough here, given that writes are always synchronized on the lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like AtomicBoolean and the explicit CAS it has, but it doesn't add much here, so I went ahead and moved to volatile.
@drcrallen can we name the artifact id to something like |
@b-slim renamed to |
I am not sure i can see the change or the comment it self
|
@b-slim I see the change. Hard refresh the webpage? |
Yes i do now.
|
|
||
## Configuration | ||
<div class="note caution"> | ||
Static configuration is no longer supported. Only cluster wide configuration is supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is too wild of a change, I can copy the old extension over and deprecate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(probably separate from this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool sounds good
minor comments but overall 👍 |
* Also add serde test
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.