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 2020/08/22 03:13:01 UTC
[GitHub] [druid] himanshug opened a new pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
himanshug opened a new pull request #10309:
URL: https://github.com/apache/druid/pull/10309
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 to `PasswordProvider` . This patch introduces `DynamicConfigProvider` as proposed in #9351 except it is not named `CredentialsProvider` 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 with `DynamicConfigProvider`
<hr>
This PR has:
- [x] 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/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] 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.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `DynamicConfigProvider`
* `KafkaRecordSupplier`
----------------------------------------------------------------
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] abhishekagarwal87 commented on a change in pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10309:
URL: https://github.com/apache/druid/pull/10309#discussion_r475253736
##########
File path: core/src/main/java/org/apache/druid/metadata/DynamicConfigProvider.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+import java.util.Map;
+
+/**
+ * This is used to get [secure] configuration in various places in an extensible way.
+ */
+@ExtensionPoint
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = MapBasedDynamicConfigProvider.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "map", value = MapBasedDynamicConfigProvider.class),
+})
+public interface DynamicConfigProvider
+{
+ Map<String, String> getConfig();
Review 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?
----------------------------------------------------------------
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] jihoonson commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-740798276
@himanshug no worries, thank you!
----------------------------------------------------------------
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] stale[bot] commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-723501438
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.
----------------------------------------------------------------
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] jihoonson commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-740286035
@himanshug I missed in my previous review that documentation is missing. Would you please add some?
----------------------------------------------------------------
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] himanshug commented on a change in pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #10309:
URL: https://github.com/apache/druid/pull/10309#discussion_r476789851
##########
File path: core/src/main/java/org/apache/druid/metadata/DynamicConfigProvider.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+import java.util.Map;
+
+/**
+ * This is used to get [secure] configuration in various places in an extensible way.
+ */
+@ExtensionPoint
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = MapBasedDynamicConfigProvider.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "map", value = MapBasedDynamicConfigProvider.class),
+})
+public interface DynamicConfigProvider
+{
+ Map<String, String> getConfig();
Review 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 still be able to work without typecasting but interface would still serve use case of any value types.
----------------------------------------------------------------
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] himanshug commented on a change in pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #10309:
URL: https://github.com/apache/druid/pull/10309#discussion_r476789851
##########
File path: core/src/main/java/org/apache/druid/metadata/DynamicConfigProvider.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+import java.util.Map;
+
+/**
+ * This is used to get [secure] configuration in various places in an extensible way.
+ */
+@ExtensionPoint
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = MapBasedDynamicConfigProvider.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "map", value = MapBasedDynamicConfigProvider.class),
+})
+public interface DynamicConfigProvider
+{
+ Map<String, String> getConfig();
Review 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.
----------------------------------------------------------------
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] himanshug merged pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug merged pull request #10309:
URL: https://github.com/apache/druid/pull/10309
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10309:
URL: https://github.com/apache/druid/pull/10309#discussion_r534373581
##########
File path: core/src/main/java/org/apache/druid/metadata/DynamicConfigProvider.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+import java.util.Map;
+
+/**
+ * This is used to get [secure] configuration in various places in an extensible way.
+ */
+@ExtensionPoint
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = MapStringDynamicConfigProvider.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "mapString", value = MapStringDynamicConfigProvider.class),
+})
+public interface DynamicConfigProvider<T>
Review comment:
nit: should we annotate `PasswordProvider` as `@Deprecated` in favor of this class?
----------------------------------------------------------------
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] himanshug commented on a change in pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #10309:
URL: https://github.com/apache/druid/pull/10309#discussion_r534549161
##########
File path: core/src/main/java/org/apache/druid/metadata/DynamicConfigProvider.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+import java.util.Map;
+
+/**
+ * This is used to get [secure] configuration in various places in an extensible way.
+ */
+@ExtensionPoint
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = MapStringDynamicConfigProvider.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "mapString", value = MapStringDynamicConfigProvider.class),
+})
+public interface DynamicConfigProvider<T>
Review 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.
----------------------------------------------------------------
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] himanshug commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-724220843
not stale
----------------------------------------------------------------
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] himanshug commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-740316011
@jihoonson sorry, I forgot about that it should be immediately useful for anyone doing similar things... will add the docs tomorrow most likely .
----------------------------------------------------------------
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] himanshug commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
himanshug commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-737554936
@clintropolis @jihoonson thanks for reviewing.
----------------------------------------------------------------
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] stale[bot] commented on pull request #10309: introduce DynamicConfigProvider interface and make kafka consumer props extensible
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10309:
URL: https://github.com/apache/druid/pull/10309#issuecomment-724220915
This issue is no longer marked as stale.
----------------------------------------------------------------
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