Skip to content
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

KAFKA-8550: Fix plugin loading of aliased converters in Connect #6959

Merged
merged 2 commits into from Aug 11, 2019

Conversation

@C0urante
Copy link
Contributor

commented Jun 18, 2019

Jira

Summary of issue: connector validation fails if an alias is used for the converter since the validation for that is done via ConfigDef.validateAll(...), which in turn invokes Class.forName(...) on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from Class.forName(...) whose name differs than the argument provided.

Summary of fix: alter ConfigDef.parseType(...) to first invoke ClassLoader.loadClass(...) on the class in order to get a handle on the actual class object to be loaded, then invoke Class.forName(...) with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of Class.forName(...) is necessary in order to allow static initialization to take place; simply calling ClassLoader.loadClass(...) is insufficient.

Summary of testing: tested manually on trunk. Added unit test to ConfigDefTest that simulates the plugin-aliasing behavior of the DelegatingClassLoader and then invokes ConfigDef.parseType on an aliased class; this test fails on the current trunk but succeeds with the changes proposed here.

This should be backported at least to 2.0; it's likely the issue goes back further than that but since it's been around for so long and has yet to be noted by anyone else it doesn't seem worth the effort to backport that much further if there are any significant merge conflicts.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
@C0urante

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@kkonstantine @rayokota could one or both of you take a look at this when you have a chance?

@rayokota
Copy link
Contributor

left a comment

LGTM

@C0urante

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Thanks @rayokota!

@rhauch could you take a look at this when you get a chance?

@rhauch

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

retest this please

@rhauch

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@C0urante, it looks like the test failure is related to this PR:

java.lang.NullPointerException
	at org.apache.kafka.common.config.ConfigDef.parseType(ConfigDef.java:718)
	at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:473)
	at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:466)
@C0urante

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Thanks for the catch, @rhauch. Turns out the AbstractConfig test was broken by the changes I made. I've altered it and ran the unit tests for the clients project again and everything's passing now.

@rhauch
rhauch approved these changes Jul 29, 2019
Copy link
Contributor

left a comment

LGTM. Thanks for fixing this issue, @C0urante!

I'll wait for a green build before merging.

@C0urante

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@rhauch it looks like the test failure on Scala 2.11 is unrelated and probably an environmental issue:

21:27:40 kafka.api.DelegationTokenEndToEndAuthorizationTest > testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl STARTED
21:27:58 kafka.api.DelegationTokenEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl failed, log available in /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk8-scala2.11/core/build/reports/testOutput/kafka.api.DelegationTokenEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl.test.stdout
21:27:58 
21:27:58 kafka.api.DelegationTokenEndToEndAuthorizationTest > testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl FAILED
21:27:58     org.scalatest.exceptions.TestFailedException: Consumed 0 records before timeout instead of the expected 1 records
@C0urante

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Huh. I saw two green builds and one red yesterday, but now I see three red builds. I don't have access to the build reports on Jenkins so I can't see what caused them. @rhauch any thoughts on how to proceed?

@rhauch rhauch merged commit 7511ccf into apache:trunk Aug 11, 2019

0 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE 11716 tests run, 67 skipped, 1 failed.
Details
JDK 11 and Scala 2.13 FAILURE 11716 tests run, 67 skipped, 1 failed.
Details
JDK 8 and Scala 2.11 FAILURE 11716 tests run, 67 skipped, 1 failed.
Details
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
rhauch added a commit that referenced this pull request Aug 11, 2019
KAFKA-8550: Fix plugin loading of aliased converters in Connect (#6959)
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
@rhauch

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

The failures were likely unrelated. I ran a build locally with multiple JDKs, and the only failures I had were unrelated.

@rhauch

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

restest this please

@C0urante C0urante deleted the C0urante:KAFKA-8550 branch Aug 19, 2019

xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [04cf8fb] KAFKA-8550: Fix plugin loading of aliased …
…converters in Connect (apache#6959)

TICKET = KAFKA-8550
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [04cf8fb]
ORIGINAL_DESCRIPTION =

Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided.

This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient.

Also corrected a unit test that relied upon the old behavior.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
(cherry picked from commit 04cf8fb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.