-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems reasonable 👍
@JsonSubTypes(value = { | ||
@JsonSubTypes.Type(name = "mapString", value = MapStringDynamicConfigProvider.class), | ||
}) | ||
public interface DynamicConfigProvider<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we annotate PasswordProvider
as @Deprecated
in favor of this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.servers
using user specific extensible mechanisms similar toPasswordProvider
. This patch introducesDynamicConfigProvider
as proposed in #9351 except it is not namedCredentialsProvider
so as to make it useful for dynamic/extensible user configuration in addition to credentials.At some point, fixing #9351 should lead to removal of
PasswordProvider
and replace all its usage withDynamicConfigProvider
This PR has:
Key changed/added classes in this PR
DynamicConfigProvider
KafkaRecordSupplier