-
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] Query time lookup cluster wide config #1576
Conversation
@@ -46,4 +46,6 @@ public Void insertOrUpdate( | |||
public void createTaskTables(); | |||
|
|||
public void createAuditTable(); | |||
|
|||
public void createNamespacesTable(); |
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'm considering moving this to a module, so don't get caught up on it being in mainline druid
8e6d2b5
to
2cd6e31
Compare
@@ -234,17 +238,17 @@ | |||
<dependency> | |||
<groupId>com.google.inject</groupId> | |||
<artifactId>guice</artifactId> | |||
<version>4.0-beta</version> | |||
<version>4.0</version> |
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.
Will remove these version changes shortly.
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.
Done
5e1ed09
to
ab97768
Compare
ab97768
to
6b29191
Compare
6b29191
to
34eff84
Compare
34eff84
to
e5f02d0
Compare
Closing for now |
|
||
|
||
# Lookups Dynamic Config (EXPERIMENTAL) | ||
Thse configuration options control the behavior of the Lookup dynamic configuration described in the [lookups page](../querying/lookups.html) |
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.
s/Thse/These/g
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
have finished reviewing this PR -
I am 👍 on this and think this can be merged once 1 is taken care of unless @b-slim has any more comments. |
2c7a90e
to
70905c5
Compare
Squashed |
@@ -26,7 +26,7 @@ | |||
<parent> | |||
<groupId>io.druid</groupId> | |||
<artifactId>druid</artifactId> | |||
<version>0.9.1-SNAPSHOT</version> | |||
<version>0.9.0-SNAPSHOT</version> |
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'll undo these, give me a sec
0211eef
to
2f36d73
Compare
LifecycleModule.register(binder, LookupReferencesManager.class); | ||
JsonConfigProvider.bind(binder, PROPERTY_BASE, LookupListeningAnnouncerConfig.class); | ||
Jerseys.addResource(binder, LookupListeningResource.class); | ||
LifecycleModule.register(binder, LookupReferencesManager.class); |
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.
duplicated LifecycleModule.register(binder, LookupReferencesManager.class)
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.
good call, artifact of merging module definitions. Fixed
2f36d73
to
5da9a28
Compare
👍 please make sure to have the issue linked to the future improvement especially about initialization. |
[QTL] Query time lookup cluster wide config
This is the next PR for QTL where cluster wide configuration is possible.
This approach uses POSTs from the coordinator to populate QTL information in all interested nodes. Interested nodes "register" via the Announcer (currently ZK based). But all communication occurs through HTTP directly between the coordinator and the node.
The PR lists over 4k lines, but a LOT of that is tests.
TODO:Ready to mergeProposal for coordinator and node endpointsSquash (Leaving change log while reviews are occurring)DocsPunted until extensions are updatedMigrate coordinator code to a module.Figure out a better way to handle the listening handling. (see Add listening-announcer extension #2286 )added in this PR in coreMigrate lookup config to auditable config type.Rebase on Promoting LookupExtractor state and LookupExtractorFactory to be a first class druid state object. #2291Move core stuff and extension stuff to its own commitMake sure put functions as expected. (I don't think deleting actually works in this version)Make sure initialization behaves as expectedActually run the thing locallyReconcile need for static config vs dynamic config. Ex: what happens if someone requests a delete or get of a lookup defined in runtime.properties?No static configMore unit tests for delete funcitonMake timeouts configurabledoneDocument historical node endpoints as advanced featureDocument experimental update aspectAdded javadoc forreplaces
addition toLookupExtractorFactory
KNOWN ISSUES:
Currently if someone manually goes in and modifies lookups on a lookup node, the cluster wide config will attempt to re-populate that node. The way to get around this behavior is to add the lookups to the node independent of cluster-wide config.
The lookup coordinator does try to delete lookups that were removed from the config.
THERE IS A MAJOR CAVEAT HERE: If a node's network is disconnected during this delete (or it is otherwise temporarily unavailable) there is no good way for the coordinator to know that, when it can communicate with the node again, if the
some_lookup
is has is supposed to be there as a custom config, or ifsome_lookup
is there from a failed delete.Such an occurrence should be RARE in the current IMPL though.