You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2024/07/16 22:28:38 UTC

[PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

gharris1727 opened a new pull request, #16608:
URL: https://github.com/apache/kafka/pull/16608

   There are a handful of places where we call Class.forName in the code base. Previously, the ConfigDef call-site required a patch to resolve aliased classes (a feature in Connect). A second call-site via AbstractHerder -> Utils.newInstance has appeared as a regression in the current release. We can apply the same fix to all other call sites for Class.forName to prevent this from happening accidentally again. If aliases are not in-use, this should be a no-op change.
   
   I added a PluginsTest case that exercises all of the ways we could construct Converter objects and ensures that they don't throw a ClassNotFoundException. This test failed before making the fix.
   
   ### 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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2234570215

   Test failures look unrelated, and the tests pass for me locally.


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2231940515

   Alternative to this PR are:
   
   * Revert #14309 -> viable but removes the new validation functionality
   * Change the validateConverterConfig to call into plugins -> more involved, and risks breaking classloading-order-preference.


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2234031483

   @gharris1727 FYI, I've opened https://github.com/gharris1727/kafka/pull/1 to add an integration test for plugin aliases. You are welcome to include it in this PR, or if you'd prefer I can file a PR against trunk to add the test instead.


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2232883147

   @gharris1727 The build wasn't working because of a `spotless` violation:
   ```
   * What went wrong:
   
   [2024-07-16T22:42:43.930Z] Execution failed for task ':connect:runtime:spotlessJavaCheck'.
   
   [2024-07-16T22:42:43.930Z] > The following files had format violations:
   
   [2024-07-16T22:42:43.930Z]       src/test/java/org/apache/kafka/connect/runtime/isolation/SynchronizationTest.java
   
   [2024-07-16T22:42:43.930Z]           @@ -24,8 +24,8 @@
   
   [2024-07-16T22:42:43.930Z]            import·org.apache.kafka.common.config.ConfigDef.Type;
   
   [2024-07-16T22:42:43.930Z]            import·org.apache.kafka.common.utils.Utils;
   
   [2024-07-16T22:42:43.930Z]            import·org.apache.kafka.connect.runtime.WorkerConfig;
   
   [2024-07-16T22:42:43.930Z]           +import·org.apache.kafka.connect.storage.Converter;
   
   [2024-07-16T22:42:43.930Z]            
   
   [2024-07-16T22:42:43.930Z]           -import·org.apache.kafka.connect.storage.Converter;
   
   [2024-07-16T22:42:43.930Z]            import·org.junit.jupiter.api.AfterEach;
   
   [2024-07-16T22:42:43.930Z]            import·org.junit.jupiter.api.BeforeEach;
   
   [2024-07-16T22:42:43.930Z]            import·org.junit.jupiter.api.Test;
   
   [2024-07-16T22:42:43.930Z]   Run './gradlew :connect:runtime:spotlessApply' to fix these violations.
   ```


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2231925833

   Also @yashmayya @mimaison and anyone else interested in 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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on code in PR #16608:
URL: https://github.com/apache/kafka/pull/16608#discussion_r1680766710


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/SynchronizationTest.java:
##########
@@ -22,8 +22,10 @@
 import org.apache.kafka.common.config.ConfigDef;
 import org.apache.kafka.common.config.ConfigDef.Importance;
 import org.apache.kafka.common.config.ConfigDef.Type;
+import org.apache.kafka.common.utils.Utils;
 import org.apache.kafka.connect.runtime.WorkerConfig;
 
+import org.apache.kafka.connect.storage.Converter;

Review Comment:
   I think this import is in the "wrong" position according to spotless



-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2234151818

   @C0urante Thanks for implementing an integration test! I think one may be warranted here because it's regressed twice now, and an integration test would have caught this.
   
   I'd prefer to unblock the release quickly, so please retarget that PR at upstream/trunk, 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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #16608:
URL: https://github.com/apache/kafka/pull/16608#discussion_r1680198269


##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -425,7 +425,14 @@ public static <T> T newInstance(String klass, Class<T> base) throws ClassNotFoun
      * @return the new class
      */
     public static <T> Class<? extends T> loadClass(String klass, Class<T> base) throws ClassNotFoundException {
-        return Class.forName(klass, true, Utils.getContextOrKafkaClassLoader()).asSubclass(base);
+        ClassLoader contextOrKafkaClassLoader = Utils.getContextOrKafkaClassLoader();
+        // Use loadClass here instead of Class.forName because the name we use here may be an alias

Review Comment:
   Aliasing is a general concept, with one particular implementation in connect. I don't think the word alias is incorrect to use here. The comment also doesn't mention plugins at all.
   
   You're right that the only time we use aliases is in connect, via the DelegatingClassLoader, so most of the time this change has no effect, loadedClass.getName().equals(klass) is true.



-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2232572839

   I'm retriggering CI as 2 jobs failed in the first run.


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on code in PR #16608:
URL: https://github.com/apache/kafka/pull/16608#discussion_r1681002975


##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/SynchronizationTest.java:
##########
@@ -22,8 +22,10 @@
 import org.apache.kafka.common.config.ConfigDef;
 import org.apache.kafka.common.config.ConfigDef.Importance;
 import org.apache.kafka.common.config.ConfigDef.Type;
+import org.apache.kafka.common.utils.Utils;
 import org.apache.kafka.connect.runtime.WorkerConfig;
 
+import org.apache.kafka.connect.storage.Converter;

Review Comment:
   ```suggestion
   import org.apache.kafka.connect.storage.Converter;
   
   ```



-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #16608:
URL: https://github.com/apache/kafka/pull/16608#issuecomment-2231925358

   Hi @C0urante @jlprat PTAL, 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.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16608:
URL: https://github.com/apache/kafka/pull/16608#discussion_r1680190967


##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -425,7 +425,14 @@ public static <T> T newInstance(String klass, Class<T> base) throws ClassNotFoun
      * @return the new class
      */
     public static <T> Class<? extends T> loadClass(String klass, Class<T> base) throws ClassNotFoundException {
-        return Class.forName(klass, true, Utils.getContextOrKafkaClassLoader()).asSubclass(base);
+        ClassLoader contextOrKafkaClassLoader = Utils.getContextOrKafkaClassLoader();
+        // Use loadClass here instead of Class.forName because the name we use here may be an alias

Review Comment:
   Those comments about "alias" seem to valid for connector only, right? I mean the plugin-able configs in other module (core, storage, and tool) have no mechanism about "alias". If the answer is YES, maybe we can tweak the comments so as to avoid incorrect assumption that "all" plugin-able configs can use "alias".



-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 merged PR #16608:
URL: https://github.com/apache/kafka/pull/16608


-- 
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

Re: [PR] KAFKA-17150: Use Utils.loadClass instead of Class.forName to resolve aliases correctly [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16608:
URL: https://github.com/apache/kafka/pull/16608#discussion_r1680270710


##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -425,7 +425,14 @@ public static <T> T newInstance(String klass, Class<T> base) throws ClassNotFoun
      * @return the new class
      */
     public static <T> Class<? extends T> loadClass(String klass, Class<T> base) throws ClassNotFoundException {
-        return Class.forName(klass, true, Utils.getContextOrKafkaClassLoader()).asSubclass(base);
+        ClassLoader contextOrKafkaClassLoader = Utils.getContextOrKafkaClassLoader();
+        // Use loadClass here instead of Class.forName because the name we use here may be an alias

Review Comment:
   I assumed the "alias" are applied to all plugin configs when reading those comments, so the origin comment is "we have some deprecated implementations having same alias as new one. Bababa..."
   
   There is no strong reason to tweak the comments, because the root cause is probably my poor reading 😢 



-- 
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