You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/09/22 14:18:04 UTC
[GitHub] [kafka] scholzj opened a new pull request #11352: Add DirectoryConfigProvider to the service provider list
scholzj opened a new pull request #11352:
URL: https://github.com/apache/kafka/pull/11352
Following the discussion in confluentinc/kafka-images#102, it seems that the `DirectoryConfigProvider` should be also listed in the list of the service providers.
CC: @C0urante
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#issuecomment-928891066
Failing tests are unrelated.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#issuecomment-928891066
Failing tests are unrelated.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley merged pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley merged pull request #11352:
URL: https://github.com/apache/kafka/pull/11352
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley merged pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley merged pull request #11352:
URL: https://github.com/apache/kafka/pull/11352
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#discussion_r716648048
##########
File path: clients/src/test/java/org/apache/kafka/common/config/provider/FileConfigProviderTest.java
##########
@@ -87,6 +89,13 @@ public void testNullPathWithKey() throws Exception {
assertNull(configData.ttl());
}
+ @Test
+ public void testServiceLoaderDiscovery() {
+ ServiceLoader<ConfigProvider> serviceLoader = ServiceLoader.load(ConfigProvider.class);
+ assertTrue(StreamSupport.stream(serviceLoader.spliterator(), false).anyMatch(sl -> sl instanceof FileConfigProvider));
+
Review comment:
Blank line
##########
File path: clients/src/test/java/org/apache/kafka/common/config/provider/DirectoryConfigProviderTest.java
##########
@@ -145,5 +147,11 @@ public void testNullPathWithKey() {
assertTrue(configData.data().isEmpty());
assertNull(configData.ttl());
}
+
+ @Test
+ public void testServiceLoaderDiscovery() {
+ ServiceLoader<ConfigProvider> serviceLoader = ServiceLoader.load(ConfigProvider.class);
+ assertTrue(StreamSupport.stream(serviceLoader.spliterator(), false).anyMatch(sl -> sl instanceof DirectoryConfigProvider));
Review comment:
`sl` -> `configProvider`
##########
File path: clients/src/test/java/org/apache/kafka/common/config/provider/FileConfigProviderTest.java
##########
@@ -87,6 +89,13 @@ public void testNullPathWithKey() throws Exception {
assertNull(configData.ttl());
}
+ @Test
+ public void testServiceLoaderDiscovery() {
+ ServiceLoader<ConfigProvider> serviceLoader = ServiceLoader.load(ConfigProvider.class);
+ assertTrue(StreamSupport.stream(serviceLoader.spliterator(), false).anyMatch(sl -> sl instanceof FileConfigProvider));
Review comment:
`sl` -> `configProvider`
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#issuecomment-926485990
I think we should add a line to the class javadoc for `ConfigProvider`, because the requirement for `ServiceLoader` discoverability doesn't seem to be documented (at least, that I know of) and is easily missed.
Also, we should add a test for discovering via `ServiceLoader`. As well as regression, people are likely to look at these classes and tests when writing their own config providers, so having a test for it is more likely to ensure they don't make the same mistake.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#discussion_r716427166
##########
File path: clients/src/test/java/org/apache/kafka/common/config/provider/DirectoryConfigProviderTest.java
##########
@@ -145,5 +146,21 @@ public void testNullPathWithKey() {
assertTrue(configData.data().isEmpty());
assertNull(configData.ttl());
}
+
+ @Test
+ public void testServiceLoaderDiscovery() {
+ ServiceLoader<ConfigProvider> serviceLoader = ServiceLoader.load(ConfigProvider.class);
+
+ boolean discovered = false;
+
+ for (ConfigProvider service : serviceLoader) {
+ System.out.println(service.getClass());
+ if (service instanceof DirectoryConfigProvider) {
+ discovered = true;
+ }
+ }
+
+ assertTrue(discovered);
Review comment:
```suggestion
assertTrue(StreamSupport.stream(serviceLoader.spliterator(), false).anyMatch(sl -> sl instanceof DirectorConfigProvider));
```
##########
File path: clients/src/main/java/org/apache/kafka/common/config/provider/ConfigProvider.java
##########
@@ -26,6 +26,8 @@
/**
* A provider of configuration data, which may optionally support subscriptions to configuration changes.
* Implementations are required to safely support concurrent calls to any of the methods in this interface.
+ * Configuration provider implementations are in Kafka Connect loaded as Service Providers using the {@code ServiceLoader} class.
+ * To support this, implementations of this interface should also contain its service provider configuration file.
Review comment:
```suggestion
* Kafka Connect discovers configuration providers using Java's Service Provider mechanism (see {@code java.util.ServiceLoader}).
* To support this, implementations of this interface should also contain a service provider configuration file in {@code META-INF/service/org.apache.kafka.common.config.provider.ConfigProvider}.
```
##########
File path: clients/src/test/java/org/apache/kafka/common/config/provider/FileConfigProviderTest.java
##########
@@ -87,6 +88,22 @@ public void testNullPathWithKey() throws Exception {
assertNull(configData.ttl());
}
+ @Test
+ public void testServiceLoaderDiscovery() {
+ ServiceLoader<ConfigProvider> serviceLoader = ServiceLoader.load(ConfigProvider.class);
+
+ boolean discovered = false;
+
+ for (ConfigProvider service : serviceLoader) {
+ System.out.println(service.getClass());
+ if (service instanceof FileConfigProvider) {
+ discovered = true;
+ }
+ }
+
+ assertTrue(discovered);
Review comment:
Same refactoring
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] scholzj commented on pull request #11352: Add DirectoryConfigProvider to the service provider list
Posted by GitBox <gi...@apache.org>.
scholzj commented on pull request #11352:
URL: https://github.com/apache/kafka/pull/11352#issuecomment-926754609
@tombentley I added the JavaDoc as well as tests for both providers.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org