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-15150: Add ServiceLoaderScanner implementation #13971

Merged

Conversation

gharris1727
Copy link
Contributor

@gharris1727 gharris1727 commented Jul 6, 2023

Add the ServiceLoaderScanner, a companion to the ReflectionScanner which does not use reflection to discover plugins.

This will be utilized later in the Plugins class for the configurable-discovery-mode startup scanning, and in the connect-plugin-path script for manifest generation. In this PR, the PluginScannerTest is parameterized to ensure that the ServiceLoaderScanner has parity on the TestPlugins. This required adding manifests for the valid TestPlugins.

Also fixes a bug where the ServiceLoader handles plugin LinkageErrors differently than Reflections. To summarize:

Before:

  • Reflections WARN logs LinkageError internally
  • ServiceLoader propagates LinkageError and kills the worker

After:

  • Reflections behaves the same as before
  • JREs with ServiceLoader implementations that make progress before throwing LinkageError:
    • PluginScanner will typically log one error per unique error message, potentially up to 100 errors.
    • If there are >100 consecutive plugins failing with LinkageError, PluginScanner propagates the LinkageError and kills the worker.
  • JREs with ServiceLoader implementations that do not make progress before throwing a LinkageError:
    • PluginScanner will typically log a single error, potentially up to 100 errors.
    • PluginScanner will propagate the LinkageError and kills the worker.

Committer Checklist (excluded from commit message)

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

@C0urante C0urante added connect kip Requires or implements a KIP labels Jul 7, 2023
@gharris1727
Copy link
Contributor Author

@C0urante Could you review this? Thanks!

Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Greg! A couple of nits and one question about testing methodology, then this should be good to go.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Contributor Author

This appears to have some JDK8 failures that look consistent that i'll have to replicate locally. I've been testing with JDK11, so it's possible that the JDK8 ServiceLoader has some slightly different behavior, since it was overhauled in JDK9.

…next

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@C0urante
Copy link
Contributor

Tested the latest commit locally with OpenJDK 8 and 17; everything passes fine. Left one last comment but once that's addressed, this is good to go. Thanks for the diligent work on this, Greg!

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
…r better log messages

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Greg. I really want to merge this but have some final thoughts. I promise this is it, though!

…tements

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (pending CI build). Thanks Greg, this was a tricky one!

…e-loader-scanner

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Contributor Author

Flaky test failures appear unrelated.

@gharris1727 gharris1727 merged commit f6e7aa3 into apache:trunk Jul 19, 2023
1 check failed
jeqo pushed a commit to jeqo/kafka that referenced this pull request Jul 21, 2023
Reviewers: Chris Egerton <chris.egerton@aiven.io>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
Reviewers: Chris Egerton <chris.egerton@aiven.io>
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
Reviewers: Chris Egerton <chris.egerton@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connect kip Requires or implements a KIP
Projects
None yet
2 participants