druid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [druid] clintropolis commented on a change in pull request #11362: Add DynamicConfigProvider for Schema Registry
Date Tue, 22 Jun 2021 09:59:14 GMT

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


Mime
View raw message