-
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 #2716
Conversation
try { | ||
Thread.sleep(10); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); |
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.
Should log that it was interrupted (probably as error) and 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.
log added
import javax.annotation.Nullable; | ||
import java.util.List; | ||
|
||
@JsonTypeName("name") |
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.
suggest making this namespace
to match io.druid.query.extraction.NamespacedExtractor
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.
updated
I realized I hadn't written this down yet, but I'm thinking this PR might be able to be the switching point for the extension to allow the recommended lookups to go through the dynamic configuration instead of through static configs for this extension. (mentioned in #2735 (comment) ) If you think such a task is too far out of scope for this PR, please let me know and we can limit the scope here to a minimal change, and have a potentially more sweeping change come later. |
@drcrallen Ok I'll try to eliminate static configuration of namespace lookup. |
@sirpkt The only druid.io modules, yes. |
… 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
7f918f3
to
14f4c88
Compare
Static config for lookup is eliminated. |
|
||
@JsonCreator | ||
public KafkaExtractionNamespace( | ||
@NotNull @JsonProperty(value = "kafkaTopic", required = true) final String kafkaTopic, | ||
@NotNull @JsonProperty(value = "namespace", required = true) final String namespace | ||
@NotNull @JsonProperty(value = "kafkaProperites", required = true) final Properties kafkaProperties |
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.
Does this need to be required? also required=true is just a hint, you still have to enforce not null checks in constructor to be ACTUALLY required.
import java.util.concurrent.ConcurrentLinkedQueue; | ||
import java.util.concurrent.ConcurrentMap; | ||
import java.util.concurrent.Executors; | ||
import java.util.concurrent.*; |
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.
could you only import the required classes ?
@sirpkt and @drcrallen what's the status of this PR ? |
@b-slim excluding the kafka extension, the rest should be about ready. |
@drcrallen and @sirpkt can we split this to make review straightforward ? |
merge sirpkt#2 and rebased. |
@b-slim this should be ready for final review (and merge) |
@b-slim can we finish this up? |
@fjy it is in the pipeline... |
*/ | ||
public class KafkaExtractionManager | ||
{ | ||
private static final Logger log = new Logger(KafkaExtractionManager.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.
can we call all the static variable with upper case please ?
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.
log
is log
across all of the Druid code base...
@drcrallen let me know when this is ready. |
@b-slim the non-kafka stuff should be ready |
@sirpkt I'm going to move this PR to a MMX branch. Since most of the reviewers are in my timezone I can have faster modifications while they are awake. |
Moved to #2926 |
@drcrallen OK |
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.
Migrated to #2926