Repository navigation
Conversation
There was a problem hiding this comment.
Should log that it was interrupted (probably as error) and return false.
There was a problem hiding this comment.
suggest making this namespace to match io.druid.query.extraction.NamespacedExtractor
|
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. |
| 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.
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.
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.
can we call all the static variable with upper case please ?
There was a problem hiding this comment.
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