introduce DynamicConfigProvider interface and make kafka consumer props extensible #10309
Conversation
| }) | ||
| public interface DynamicConfigProvider | ||
| { | ||
| Map<String, String> getConfig(); |
There was a problem hiding this comment.
should this return Map<String, Object> to support a wider range of possible configuration value types since any extension other than Kafka could use it?
There was a problem hiding this comment.
Intention of this interface certainly is to be used everywhere we get extensible user config (currently the places that are using PasswordProvider), while a cursory look says that Map<String, String> would be sufficient for all the places but I see the desire to have it be more generic. I am probably gonna parameterize the interface to DynamicConfigProvider<T> so that client code would be able to work without typecasting but interface would still serve use case of any value types.
Change-Id: I2e3e89f8617b6fe7fc96859deca4011f609dc5a3
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions. |
|
not stale |
|
This issue is no longer marked as stale. |
| @JsonSubTypes(value = { | ||
| @JsonSubTypes.Type(name = "mapString", value = MapStringDynamicConfigProvider.class), | ||
| }) | ||
| public interface DynamicConfigProvider<T> |
There was a problem hiding this comment.
nit: should we annotate PasswordProvider as @Deprecated in favor of this class?
There was a problem hiding this comment.
I didn't mark that deprecated because users wanting to have their own extension for db password must still implement a PasswordProvider ,
however, for Druid devs, any new credential/extensible-config type thing must use DynamicConfigProvider ... so PasswordProvider is "deprecated" in that sense.
will mark it deprecated and add few comments.
|
@clintropolis @jihoonson thanks for reviewing. |
|
@himanshug I missed in my previous review that documentation is missing. Would you please add some? |
|
@jihoonson sorry, I forgot about that it should be immediately useful for anyone doing similar things... will add the docs tomorrow most likely . |
|
@himanshug no worries, thank you! |
…ps extensible (apache#10309) * introduce DynamicConfigProvider interface and make kafka consumer props extensible * fix intellij inspection error * make DynamicConfigProvider generic Change-Id: I2e3e89f8617b6fe7fc96859deca4011f609dc5a3 * deprecate PasswordProvider
supersedes #9693
Description
This patch really came from need of supplying few kafka consumer properties such as
bootstrap.serversusing user specific extensible mechanisms similar toPasswordProvider. This patch introducesDynamicConfigProvideras proposed in #9351 except it is not namedCredentialsProviderso as to make it useful for dynamic/extensible user configuration in addition to credentials.At some point, fixing #9351 should lead to removal of
PasswordProviderand replace all its usage withDynamicConfigProviderThis PR has:
Key changed/added classes in this PR
DynamicConfigProviderKafkaRecordSupplier