Skip to content

MINOR: Refactor DelegatingClassLoader to emit immutable PluginScanResult#13771

Merged
C0urante merged 14 commits intoapache:trunkfrom
gharris1727:minor-plugin-scan-result
Jun 6, 2023
Merged

MINOR: Refactor DelegatingClassLoader to emit immutable PluginScanResult#13771
C0urante merged 14 commits intoapache:trunkfrom
gharris1727:minor-plugin-scan-result

Conversation

@gharris1727
Copy link
Contributor

@gharris1727 gharris1727 commented May 27, 2023

The DelegatingClassLoader has a large number of fields and methods for keeping track of known PluginDesc objects. It has this in common with the PluginScanResult data object, which has a similar set of fields and methods. On trunk, the PluginScanResult is only used as a return value of one internal function, before it's results are accumulated by the DCL and discarded. To simplify the DCL, we should use the PluginScanResult object as a container to store and accumulate PluginDesc objects, and return it to the caller to allow them to inspect the scanned plugins without interacting with the DCL.

Using the PluginScanResult as an accumulator, we can delay writing scan results to the pluginLoaders and aliases fields until after all scanning has taken place. This is done via the new installDiscoveredPlugins method. This prevents plugins being scanned from having an inconsistent delegation path: Previously, because the accumulators were updated as scanning proceeded, plugins may or may not be able to see one another via the DelegatingClassLoader::loadClass method. Now, plugins will not be able to see one another before installDiscoveredPlugins is applied and scanning is finished.

This is a first-pass refactor, before pulling the scanning logic out of the DCL to further simplify it. Once external scanning is complete, the caller will call installDiscoveredPlugins to finish initialization of the DCL.

In the trunk implementation, there is some order-sensitivity to the scanning process because of the accumulator fields. In particular:

  1. Because the classpath is scanned last, plugins on the classpath automatically take precedence over isolated plugins. In order to replicate this effect, PluginDesc objects now explicitly compare their isolated/non-isolated nature, and order classpath plugins first.
  2. The alias mechanism attempts to determine if aliases are unique, but on trunk it allows colliding aliases from different plugin types to overwrite one another. This is changed to prevent alias collisions across plugin types, now that the iteration over PluginDesc objects isn't grouped by plugin type.

Committer Checklist (excluded from commit message)

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

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. Some minor stylistic comments. Also not sure we should be modifying the aliasing logic without a very good reason; let me know if there's implications that I'm missing.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
…l class name.

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

C0urante commented Jun 2, 2023

Thanks Greg! I think the tradeoffs here are acceptable. Can we add some testing coverage for PluginUtils::computeAliases? Once that's done this should be good to merge.

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

Here's an example of the new log message, pulled from the test run:

[2023-06-02 10:15:57,932] WARN Ambiguous alias 'Colliding' refers to multiple distinct plugins [org.apache.kafka.connect.runtime.isolation.PluginUtilsTest$CollidingHeaderConverter, org.apache.kafka.connect.runtime.isolation.PluginUtilsTest$CollidingConverter]: ignoring (org.apache.kafka.connect.runtime.isolation.PluginUtils:378)

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! Really appreciate the added testing coverage for alias computation. Some small test comments and one logging suggestion and 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>
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! LGTM

Will merge pending CI build

@C0urante
Copy link
Contributor

C0urante commented Jun 5, 2023

@gharris1727 there are test failures in the latest CI run. Can you look into those? Feel free to just rebase on the latest trunk if that's all that's required.

@C0urante
Copy link
Contributor

C0urante commented Jun 6, 2023

Test failures appear unrelated (there is a known bug in the reflections library that we use that caused a test failure which may appear related, but is not). Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants