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 15291 connect plugins implement versioned #14159
Kafka 15291 connect plugins implement versioned #14159
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aindriu-aiven Thanks for working on this!
The migration looks good for all of the plugins you've addressed, and thanks for adding corresponding tests.
Unfortunately the StringConverter is one of the most used plugins, so it probably shouldn't be left out of the migration.
Instead of picking one of these user-facing plugins to be un-Versioned, we can use one of the TestPlugins
which are intended for testing the isolation infrastructure. Can you add a test to PluginScannerTest
which asserts that an un-Versioned plugin is scanned to use PluginDesc.UNDEFINED_VERSION
?
I'm also aware of the ConnectorPluginsResourceTest
which handles some of these plugins and injects the PluginDesc.UNDEFINED_VERSION
for them currently. Can you update that test to provide the appVersion for the plugins you've migrated?
connect/transforms/src/test/java/org/apache/kafka/connect/transforms/TimestampRouterTest.java
Show resolved
Hide resolved
…onverter, HeaderConverter, Transformation, Predicate, and ConnectorClientConfigOverridePolicy Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
…y inherited, leave StringConverter un-implemented to test undefined version Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
…rent had already implemented it Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
…ed behaviour to PluginScannerTest and also update ConnectorPluginsResourceTest to have the correct version information for each updated plugin. Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
2fff661
to
1ae5cc9
Compare
Thanks @gharris1727 I think I covered all the points you mentioned there any issues at all though please let me know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Testing this with the un-merged connect-plugin-path tool shows that all of the plugins have versions now.
I just had some suggestions for the test.
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java
Outdated
Show resolved
Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java
Outdated
Show resolved
Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java
Outdated
Show resolved
Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginScannerTest.java
Outdated
Show resolved
Hide resolved
…/isolation/PluginScannerTest.java Commit more descriptive test name Co-authored-by: Greg Harris <gharris1727@gmail.com>
…/isolation/PluginScannerTest.java Commit more descriptive test name Co-authored-by: Greg Harris <gharris1727@gmail.com>
…th and also adding a beforeClass work around for a circular dependency in TestPlugins Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks @aindriu-aiven for the improvement!
Flaky test failures appear unrelated, and tests pass locally. |
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io> Reviewers: Andrew Schofield, Greg Harris <greg.harris@aiven.io>
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io> Reviewers: Andrew Schofield, Greg Harris <greg.harris@aiven.io>
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io> Reviewers: Andrew Schofield, Greg Harris <greg.harris@aiven.io>
As Per https://issues.apache.org/jira/projects/KAFKA/issues/KAFKA-15291
Following https://issues.apache.org/jira/browse/KAFKA-14863 the plugin scanning logic was updated to allow plugins to opt-in to the Versioned interface individually.
All subclasses of Coverter, HeaderConverter, Transformation, Predicate and ConnectorClientConfigOverridePolicy have been updated to implement Versioned.
Unit tests have been added to ensure that the AppInfoParser.getVersion() is being called to provide the relevant version.
Existing test classes implementing the subclasses have also been updated to ensure that the addition of versioned does not impact these test cases.
StringConverter has been left un implemented so that the UNDEFINED_VERSION behaviour can be tested.
Committer Checklist (excluded from commit message)