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/02/01 03:09:49 UTC
[GitHub] [kafka] chia7712 opened a new pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
chia7712 opened a new pull request #10010:
URL: https://github.com/apache/kafka/pull/10010
reference to https://github.com/apache/kafka/pull/10006#issuecomment-770394615
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#discussion_r567662275
##########
File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
##########
@@ -276,52 +277,46 @@ public RestrictedClassLoader() {
ClassLoader restrictedClassLoader = new RestrictedClassLoader();
ClassLoader defaultClassLoader = AbstractConfig.class.getClassLoader();
- // Test default classloading where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test default classloading where default classes are not visible to thread context classloader
- // Static classloading is used for default classes, so instance creation should succeed.
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test class overrides with names or classes where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig.testOverrides();
-
- // Test class overrides with names or classes where all classes are visible to Kafka classloader, context classloader is null
- Thread.currentThread().setContextClassLoader(null);
- ClassTestConfig.testOverrides();
-
- // Test class overrides where some classes are not visible to thread context classloader
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- // Properties specified as classes should succeed
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Collections.singletonList(ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Arrays.asList(ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- // Properties specified as classNames should fail to load classes
+ ClassLoader origin = Thread.currentThread().getContextClassLoader();
try {
- new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS.getName(), null);
- fail("Config created with class property that cannot be loaded");
Review comment:
It's good refactor to use `assertThrow`, but the failed message is missed. Please add it in `assertThrow`. Thanks.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#issuecomment-770703994
@showuon Thanks for your reviews. I have addressed your comment by https://github.com/apache/kafka/pull/10010/commits/dac031f228c8e8db02de2f17e14f43a96db11b8e
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#discussion_r567662443
##########
File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
##########
@@ -276,52 +277,46 @@ public RestrictedClassLoader() {
ClassLoader restrictedClassLoader = new RestrictedClassLoader();
ClassLoader defaultClassLoader = AbstractConfig.class.getClassLoader();
- // Test default classloading where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test default classloading where default classes are not visible to thread context classloader
- // Static classloading is used for default classes, so instance creation should succeed.
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test class overrides with names or classes where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig.testOverrides();
-
- // Test class overrides with names or classes where all classes are visible to Kafka classloader, context classloader is null
- Thread.currentThread().setContextClassLoader(null);
- ClassTestConfig.testOverrides();
-
- // Test class overrides where some classes are not visible to thread context classloader
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- // Properties specified as classes should succeed
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Collections.singletonList(ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Arrays.asList(ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- // Properties specified as classNames should fail to load classes
+ ClassLoader origin = Thread.currentThread().getContextClassLoader();
try {
- new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS.getName(), null);
- fail("Config created with class property that cannot be loaded");
- } catch (ConfigException e) {
- // Expected Exception
- }
- try {
- testConfig = new ClassTestConfig(null, Arrays.asList(ClassTestConfig.VISIBLE_CLASS.getName(), ClassTestConfig.RESTRICTED_CLASS.getName()));
- testConfig.getConfiguredInstances("list.prop", MetricsReporter.class);
- fail("Should have failed to load class");
- } catch (KafkaException e) {
- // Expected Exception
- }
- try {
- testConfig = new ClassTestConfig(null, ClassTestConfig.VISIBLE_CLASS.getName() + "," + ClassTestConfig.RESTRICTED_CLASS.getName());
- testConfig.getConfiguredInstances("list.prop", MetricsReporter.class);
- fail("Should have failed to load class");
Review comment:
same here
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 merged pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10010:
URL: https://github.com/apache/kafka/pull/10010
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 merged pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10010:
URL: https://github.com/apache/kafka/pull/10010
----------------------------------------------------------------
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
[GitHub] [kafka] hachikuji commented on a change in pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#discussion_r568270665
##########
File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
##########
@@ -276,52 +277,49 @@ public RestrictedClassLoader() {
ClassLoader restrictedClassLoader = new RestrictedClassLoader();
ClassLoader defaultClassLoader = AbstractConfig.class.getClassLoader();
- // Test default classloading where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test default classloading where default classes are not visible to thread context classloader
- // Static classloading is used for default classes, so instance creation should succeed.
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test class overrides with names or classes where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig.testOverrides();
-
- // Test class overrides with names or classes where all classes are visible to Kafka classloader, context classloader is null
- Thread.currentThread().setContextClassLoader(null);
- ClassTestConfig.testOverrides();
-
- // Test class overrides where some classes are not visible to thread context classloader
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- // Properties specified as classes should succeed
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Collections.singletonList(ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Arrays.asList(ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- // Properties specified as classNames should fail to load classes
+ ClassLoader origin = Thread.currentThread().getContextClassLoader();
Review comment:
nit: maybe `originalClassLoader` to be consistent with other names?
----------------------------------------------------------------
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
[GitHub] [kafka] hachikuji commented on a change in pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#discussion_r568270665
##########
File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
##########
@@ -276,52 +277,49 @@ public RestrictedClassLoader() {
ClassLoader restrictedClassLoader = new RestrictedClassLoader();
ClassLoader defaultClassLoader = AbstractConfig.class.getClassLoader();
- // Test default classloading where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test default classloading where default classes are not visible to thread context classloader
- // Static classloading is used for default classes, so instance creation should succeed.
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test class overrides with names or classes where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig.testOverrides();
-
- // Test class overrides with names or classes where all classes are visible to Kafka classloader, context classloader is null
- Thread.currentThread().setContextClassLoader(null);
- ClassTestConfig.testOverrides();
-
- // Test class overrides where some classes are not visible to thread context classloader
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- // Properties specified as classes should succeed
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Collections.singletonList(ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Arrays.asList(ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- // Properties specified as classNames should fail to load classes
+ ClassLoader origin = Thread.currentThread().getContextClassLoader();
Review comment:
nit: maybe `originalClassLoader` to be consistent with other names?
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #10010: MINOR: AbstractConfigTest.testClassConfigs should reset the class loa…
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10010:
URL: https://github.com/apache/kafka/pull/10010#discussion_r567662357
##########
File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java
##########
@@ -276,52 +277,46 @@ public RestrictedClassLoader() {
ClassLoader restrictedClassLoader = new RestrictedClassLoader();
ClassLoader defaultClassLoader = AbstractConfig.class.getClassLoader();
- // Test default classloading where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test default classloading where default classes are not visible to thread context classloader
- // Static classloading is used for default classes, so instance creation should succeed.
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- testConfig = new ClassTestConfig();
- testConfig.checkInstances(ClassTestConfig.DEFAULT_CLASS, ClassTestConfig.DEFAULT_CLASS);
-
- // Test class overrides with names or classes where all classes are visible to thread context classloader
- Thread.currentThread().setContextClassLoader(defaultClassLoader);
- ClassTestConfig.testOverrides();
-
- // Test class overrides with names or classes where all classes are visible to Kafka classloader, context classloader is null
- Thread.currentThread().setContextClassLoader(null);
- ClassTestConfig.testOverrides();
-
- // Test class overrides where some classes are not visible to thread context classloader
- Thread.currentThread().setContextClassLoader(restrictedClassLoader);
- // Properties specified as classes should succeed
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Collections.singletonList(ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- testConfig = new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS, Arrays.asList(ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS));
- testConfig.checkInstances(ClassTestConfig.RESTRICTED_CLASS, ClassTestConfig.VISIBLE_CLASS, ClassTestConfig.RESTRICTED_CLASS);
- // Properties specified as classNames should fail to load classes
+ ClassLoader origin = Thread.currentThread().getContextClassLoader();
try {
- new ClassTestConfig(ClassTestConfig.RESTRICTED_CLASS.getName(), null);
- fail("Config created with class property that cannot be loaded");
- } catch (ConfigException e) {
- // Expected Exception
- }
- try {
- testConfig = new ClassTestConfig(null, Arrays.asList(ClassTestConfig.VISIBLE_CLASS.getName(), ClassTestConfig.RESTRICTED_CLASS.getName()));
- testConfig.getConfiguredInstances("list.prop", MetricsReporter.class);
- fail("Should have failed to load class");
Review comment:
same here
----------------------------------------------------------------
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