Add DynamicConfigProvider for Schema Registry #11362

bananaaggle · 2021-06-13T10:02:41Z

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 (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
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 is met.
added integration tests.
been tested in a test Druid cluster.

bananaaggle · 2021-06-13T10:04:49Z

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

clintropolis

👍

clintropolis · 2021-06-22T09:42:42Z

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)


accidental change?

clintropolis · 2021-06-22T09:46:55Z

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 |


I suggest linking to operations/dynamic-config-provider.md instead of providing an example inline, maybe something like

Suggested change

| 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 |

| 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)

Good suggestion! I'll change it.

clintropolis · 2021-06-22T09:53:08Z

...ions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java

  )
  {
    this.url = url;
    this.capacity = capacity == null ? Integer.MAX_VALUE : capacity;
    this.urls = urls;
    this.config = config;
    this.headers = headers;
+    this.mapper = new ObjectMapper();


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

clintropolis · 2021-06-22T09:53:37Z

.../main/java/org/apache/druid/data/input/protobuf/SchemaRegistryBasedProtobufBytesDecoder.java

  )
  {
    this.url = url;
    this.capacity = capacity == null ? Integer.MAX_VALUE : capacity;
    this.urls = urls;
    this.config = config;
    this.headers = headers;
+    this.mapper = new ObjectMapper();


same comment about injecting instead of making a new ObjectMapper

OK, I'll change it.

clintropolis · 2021-06-22T09:57:59Z

...ions/src/main/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoder.java

  {
    return headers;
  }

+  protected Map<String, String> createRegistryHeader()


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

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?

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.

I will add unit tests for it. As for Kafka, In #11372, I add DynamicConfigProvider for Kafka in producer properties. I think I can do the changes for Kafka in #11372, do you think it is suitable?

Unit tests added.

clintropolis

lgtm thanks! fix up the conflicts and I think this is ready to merge 👍

bananaaggle · 2021-07-12T12:30:38Z

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.

suneet-s · 2021-07-13T01:29:03Z

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

bananaaggle · 2021-07-13T06:57:05Z

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

Thanks!

bananaaggle · 2021-07-21T05:32:15Z

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!

clintropolis · 2021-08-03T20:24:45Z

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 👍

bananaaggle · 2021-08-04T01:39:26Z

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!

clintropolis added Area - Ingestion Area - Operations labels Jun 16, 2021

clintropolis reviewed Jun 22, 2021

View reviewed changes

clintropolis approved these changes Jul 7, 2021

View reviewed changes

yuanyi added 10 commits July 8, 2021 16:38

add_DynamicConfigProvider_for_schema_registry

b685bca

bug fixed

9acc33a

add document

eaf1cf6

fix document

42c1f16

fix spot bug

ba11f17

fix document

7adb0fa

inject ObjectMapper

bf17872

add DynamicConfigProviderUtils

254f8e7

add UT

607740b

bug fixed

9a6d0bd

bananaaggle force-pushed the add_DynamicConfigProvider_for_schema_registry branch from 006d1c7 to 9a6d0bd Compare July 8, 2021 09:06

clintropolis merged commit aa7cb50 into apache:master Aug 3, 2021

bananaaggle mentioned this pull request Aug 4, 2021

Add DynamicConfigProvider for Kafka Emitter #11372

Closed

9 tasks

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DynamicConfigProvider for Schema Registry #11362

Add DynamicConfigProvider for Schema Registry #11362

bananaaggle commented Jun 13, 2021

bananaaggle commented Jun 13, 2021

clintropolis left a comment

clintropolis Jun 22, 2021

bananaaggle Jun 22, 2021

clintropolis Jun 22, 2021

bananaaggle Jun 22, 2021

clintropolis Jun 22, 2021

clintropolis Jun 22, 2021

bananaaggle Jun 23, 2021

clintropolis Jun 22, 2021

bananaaggle Jun 23, 2021 •

edited

Loading

clintropolis Jun 26, 2021

bananaaggle Jun 26, 2021 •

edited

Loading

bananaaggle Jul 3, 2021

clintropolis left a comment

bananaaggle commented Jul 12, 2021

suneet-s commented Jul 13, 2021

bananaaggle commented Jul 13, 2021

bananaaggle commented Jul 21, 2021 •

edited

Loading

clintropolis commented Aug 3, 2021

bananaaggle commented Aug 4, 2021

	\| 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 \|
	\| config \| Json \| To send additional configurations, configured for Schema Registry. This can be supplied via a [DynamicConfigProvider](../../operations/dynamic-config-provider.md). \| no \|

Add DynamicConfigProvider for Schema Registry #11362

Add DynamicConfigProvider for Schema Registry #11362

Conversation

bananaaggle commented Jun 13, 2021

bananaaggle commented Jun 13, 2021

clintropolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bananaaggle Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bananaaggle Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment

bananaaggle commented Jul 12, 2021

suneet-s commented Jul 13, 2021

bananaaggle commented Jul 13, 2021

bananaaggle commented Jul 21, 2021 • edited Loading

clintropolis commented Aug 3, 2021

bananaaggle commented Aug 4, 2021

bananaaggle Jun 23, 2021 •

edited

Loading

bananaaggle Jun 26, 2021 •

edited

Loading

bananaaggle commented Jul 21, 2021 •

edited

Loading