You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:49:28 UTC

[GitHub] [druid] bananaaggle opened a new pull request #11362: Add DynamicConfigProvider for Schema Registry

bananaaggle opened a new pull request #11362:
URL: https://github.com/apache/druid/pull/11362


   DynamicConfigProvider is an interface for users to to get secure configuration in various places in an extensible way. This feature allow users who use Confluent schema registry can pass config and headers with this interface.
   
   This PR has:
   - [ ] been self-reviewed.
      - [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
   - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [ ] been tested in a test Druid cluster.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r659123755



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       I will add unit tests for it. As for Kafka, In [#11372](https://github.com/apache/druid/pull/11372), I add DynamicConfigProvider for Kafka in producer properties, I think I can do the changes for Kafka in this [#11372](https://github.com/apache/druid/pull/11372), do you think it is suitable? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r659123755



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       I will add unit tests for it. As for Kafka, In [#11372](https://github.com/apache/druid/pull/11372), I add DynamicConfigProvider for Kafka in producer properties. I think I can do the changes for Kafka in [#11372](https://github.com/apache/druid/pull/11372), do you think it is suitable? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle edited a comment on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle edited a comment on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-883903122


   Hi, @clintropolis. I've resolved conflicts. Can you help me merge it? I want to change DynamicConfigProviderUtils.java in [#11372](https://github.com/apache/druid/pull/11372) to adapt to kafka properties, do you think it's properly? Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] suneet-s commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-878706784


   @bananaaggle I've re-triggered what appears to be flaky tests 🤞 it passes on this run


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-878829643


   > @bananaaggle I've re-triggered what appears to be flaky tests 🤞 it passes on this run
   
   Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-878239864


   Hi, @clintropolis. I've resolved conflicts, but integration test failed. I find some 503 errors in tests and I think it caused by some network errors. Can you help me rerun them? Thanks.   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis merged pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11362:
URL: https://github.com/apache/druid/pull/11362


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-860185252


   Hi, @clintropolis, happy weekend! Follow your suggestion, I've integrate schema registry with DynamicConfigProvider.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656054607



##########
File path: docs/development/extensions-core/avro.md
##########
@@ -26,7 +26,7 @@ title: "Apache Avro"
 
 This Apache Druid extension enables Druid to ingest and understand the Apache Avro data format. This extension provides 
 two Avro Parsers for stream ingestion and Hadoop batch ingestion. 
-See [Avro Hadoop Parser](../../ingestion/data-formats.md#avro-hadoop-parser) and [Avro Stream Parser](../../ingestion/data-formats.md#avro-stream-parser)
+See [Avro Hadoop Parser](../../ingestion/data-formats.md#avro-hadoop-parser) and [Avro Stream Parser](../../ingestion/c.md#avro-stream-parser)

Review comment:
       accidental change?

##########
File path: docs/ingestion/data-formats.md
##########
@@ -380,8 +380,8 @@ For details, see the Schema Registry [documentation](http://docs.confluent.io/cu
 | url | String | Specifies the url endpoint of the Schema Registry. | yes |
 | capacity | Integer | Specifies the max size of the cache (default = Integer.MAX_VALUE). | no |
 | urls | Array<String> | Specifies the url endpoints of the multiple Schema Registry instances. | yes(if `url` is not provided) |
-| config | Json | To send additional configurations, configured for Schema Registry | no |
-| headers | Json | To send headers to the Schema Registry | no |
+| config | Json | To send additional configurations, configured for Schema Registry.  User can implement a `DynamicConfigProvider` to supply some properties at runtime, by adding `"druid.dynamic.config.provider"`:`{"type": "<registered_dynamic_config_provider_name>", ...}` in json. | no |

Review comment:
       I suggest linking to `operations/dynamic-config-provider.md` instead of providing an example inline, maybe something like 
   ```suggestion
   | config | Json | To send additional configurations, configured for Schema Registry. This can be supplied via a [DynamicConfigProvider](../../operations/dynamic-config-provider.md). | no |
   ```
   or something similar (and for the other sections that have been updated)

##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -48,27 +52,30 @@
   private final String url;
   private final int capacity;
   private final List<String> urls;
-  private final Map<String, ?> config;
-  private final Map<String, String> headers;
+  private final Map<String, Object> config;
+  private final Map<String, Object> headers;
+  private final ObjectMapper mapper;
+  public static final String DRUID_DYNAMIC_CONFIG_PROVIDER_KEY = "druid.dynamic.config.provider";
 
   @JsonCreator
   public SchemaRegistryBasedAvroBytesDecoder(
       @JsonProperty("url") @Deprecated String url,
       @JsonProperty("capacity") Integer capacity,
       @JsonProperty("urls") @Nullable List<String> urls,
-      @JsonProperty("config") @Nullable Map<String, ?> config,
-      @JsonProperty("headers") @Nullable Map<String, String> headers
+      @JsonProperty("config") @Nullable Map<String, Object> config,
+      @JsonProperty("headers") @Nullable Map<String, Object> headers
   )
   {
     this.url = url;
     this.capacity = capacity == null ? Integer.MAX_VALUE : capacity;
     this.urls = urls;
     this.config = config;
     this.headers = headers;
+    this.mapper = new ObjectMapper();

Review comment:
       I think instead of creating a new ObjectMapper this should be `@JacksonInject` as a constructor argument.

##########
File path: extensions-core/protobuf-extensions/src/main/java/org/apache/druid/data/input/protobuf/SchemaRegistryBasedProtobufBytesDecoder.java
##########
@@ -50,27 +53,30 @@
   private final String url;
   private final int capacity;
   private final List<String> urls;
-  private final Map<String, ?> config;
-  private final Map<String, String> headers;
+  private final Map<String, Object> config;
+  private final Map<String, Object> headers;
+  private final ObjectMapper mapper;
+  public static final String DRUID_DYNAMIC_CONFIG_PROVIDER_KEY = "druid.dynamic.config.provider";
 
   @JsonCreator
   public SchemaRegistryBasedProtobufBytesDecoder(
       @JsonProperty("url") @Deprecated String url,
       @JsonProperty("capacity") Integer capacity,
       @JsonProperty("urls") @Nullable List<String> urls,
-      @JsonProperty("config") @Nullable Map<String, ?> config,
-      @JsonProperty("headers") @Nullable Map<String, String> headers
+      @JsonProperty("config") @Nullable Map<String, Object> config,
+      @JsonProperty("headers") @Nullable Map<String, Object> headers
   )
   {
     this.url = url;
     this.capacity = capacity == null ? Integer.MAX_VALUE : capacity;
     this.urls = urls;
     this.config = config;
     this.headers = headers;
+    this.mapper = new ObjectMapper();

Review comment:
       same comment about injecting instead of making a new `ObjectMapper`

##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       These methods all look basically the same for both avro and protobuf (and kafka).
   
   I suggest making a static method or two, perhaps in a new class, maybe `DynamicConfigProviderUtils` or something similar in `druid-core`, that takes a `Map<String, Object>` and performs this logic




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656150771



##########
File path: docs/ingestion/data-formats.md
##########
@@ -380,8 +380,8 @@ For details, see the Schema Registry [documentation](http://docs.confluent.io/cu
 | url | String | Specifies the url endpoint of the Schema Registry. | yes |
 | capacity | Integer | Specifies the max size of the cache (default = Integer.MAX_VALUE). | no |
 | urls | Array<String> | Specifies the url endpoints of the multiple Schema Registry instances. | yes(if `url` is not provided) |
-| config | Json | To send additional configurations, configured for Schema Registry | no |
-| headers | Json | To send headers to the Schema Registry | no |
+| config | Json | To send additional configurations, configured for Schema Registry.  User can implement a `DynamicConfigProvider` to supply some properties at runtime, by adding `"druid.dynamic.config.provider"`:`{"type": "<registered_dynamic_config_provider_name>", ...}` in json. | no |

Review comment:
       Good suggestion! I'll change it. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656822580



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       Thanks for reviewing! I add a DynamicConfigProviderUtils which provides function like ```createRegistryHeader()```, but it can not adapt to kafka. Should I adapt to kafka in this PR?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656150422



##########
File path: docs/development/extensions-core/avro.md
##########
@@ -26,7 +26,7 @@ title: "Apache Avro"
 
 This Apache Druid extension enables Druid to ingest and understand the Apache Avro data format. This extension provides 
 two Avro Parsers for stream ingestion and Hadoop batch ingestion. 
-See [Avro Hadoop Parser](../../ingestion/data-formats.md#avro-hadoop-parser) and [Avro Stream Parser](../../ingestion/data-formats.md#avro-stream-parser)
+See [Avro Hadoop Parser](../../ingestion/data-formats.md#avro-hadoop-parser) and [Avro Stream Parser](../../ingestion/c.md#avro-stream-parser)

Review comment:
       Fixed.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r659088694



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       oops, it looks like my suggestion made coverage check fail because the tests that cover `DynamicConfigProviderUtils` live in a different module. If it isn't too much trouble could add a test for it and it should make it go away.
   
   I think its ok to do the changes to kafka in a follow-up if you would prefer.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r663328509



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       Unit tests added.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r659123755



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       I will add unit tests for it. As for Kafka, In [#11372](https://github.com/apache/druid/pull/11372), I add DynamicConfigProvider for Kafka in producer properties. I think I can do the changes for Kafka in this [#11372](https://github.com/apache/druid/pull/11372), do you think it is suitable? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-883903122


   Hi, @clintropolis. I've resolved conflicts. Can you help merge it? I want to change DynamicConfigProviderUtils.java in [#11372](https://github.com/apache/druid/pull/11372) to adapt to kafka properties, do you think it's properly?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656783651



##########
File path: extensions-core/protobuf-extensions/src/main/java/org/apache/druid/data/input/protobuf/SchemaRegistryBasedProtobufBytesDecoder.java
##########
@@ -50,27 +53,30 @@
   private final String url;
   private final int capacity;
   private final List<String> urls;
-  private final Map<String, ?> config;
-  private final Map<String, String> headers;
+  private final Map<String, Object> config;
+  private final Map<String, Object> headers;
+  private final ObjectMapper mapper;
+  public static final String DRUID_DYNAMIC_CONFIG_PROVIDER_KEY = "druid.dynamic.config.provider";
 
   @JsonCreator
   public SchemaRegistryBasedProtobufBytesDecoder(
       @JsonProperty("url") @Deprecated String url,
       @JsonProperty("capacity") Integer capacity,
       @JsonProperty("urls") @Nullable List<String> urls,
-      @JsonProperty("config") @Nullable Map<String, ?> config,
-      @JsonProperty("headers") @Nullable Map<String, String> headers
+      @JsonProperty("config") @Nullable Map<String, Object> config,
+      @JsonProperty("headers") @Nullable Map<String, Object> headers
   )
   {
     this.url = url;
     this.capacity = capacity == null ? Integer.MAX_VALUE : capacity;
     this.urls = urls;
     this.config = config;
     this.headers = headers;
+    this.mapper = new ObjectMapper();

Review comment:
       OK, I'll change it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-892291454


   > > Hi, @clintropolis. I've resolved conflicts. Can you help me merge it? I want to change DynamicConfigProviderUtils.java in #11372 to adapt to kafka properties, do you think it's properly? Thanks!
   > 
   > Very sorry for the delay! I took a much needed couple week break and am just getting back to this 😅, will merge now 👍
   
   Ahhhhhhh, welcome back!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle edited a comment on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle edited a comment on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-883903122


   Hi, @clintropolis. I've resolved conflicts. Can you help merge it? I want to change DynamicConfigProviderUtils.java in [#11372](https://github.com/apache/druid/pull/11372) to adapt to kafka properties, do you think it's properly? Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11362:
URL: https://github.com/apache/druid/pull/11362#issuecomment-892141192


   >Hi, @clintropolis. I've resolved conflicts. Can you help me merge it? I want to change DynamicConfigProviderUtils.java in #11372 to adapt to kafka properties, do you think it's properly? Thanks!
   
   Very sorry for the delay! I took a much needed couple week break and am just getting back to this 😅, will merge now :+1:


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] bananaaggle commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry

Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11362:
URL: https://github.com/apache/druid/pull/11362#discussion_r656822580



##########
File path: extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java
##########
@@ -91,17 +98,62 @@ public int getCapacity()
   }
 
   @JsonProperty
-  public Map<String, ?> getConfig()
+  public Map<String, Object> getConfig()
   {
     return config;
   }
 
   @JsonProperty
-  public Map<String, String> getHeaders()
+  public Map<String, Object> getHeaders()
   {
     return headers;
   }
 
+  protected Map<String, String> createRegistryHeader()

Review comment:
       Thanks for reviewing! I add a DynamicConfigProviderUtils which provides function like ```createRegistryHeader()```, but it can not adapt to kafka.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org