Skip to content

KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation#8630

Merged
kkonstantine merged 6 commits intoapache:trunkfrom
gharris1727:config-request-isolation
Jun 10, 2020
Merged

KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation#8630
kkonstantine merged 6 commits intoapache:trunkfrom
gharris1727:config-request-isolation

Conversation

@gharris1727
Copy link
Copy Markdown
Contributor

@gharris1727 gharris1727 commented May 7, 2020

This is a partner to #6796 that applied the same change but for ConnectorClientConfigOverridePolicy.

Signed-off-by: Greg Harris gregh@confluent.io

Committer Checklist (excluded from commit message)

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

@gharris1727
KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading i…
9c506f3 
…solation

Signed-off-by: Greg Harris <gregh@confluent.io>
@gharris1727
Copy link
Copy Markdown
Contributor Author

gharris1727 commented May 7, 2020

@kkonstantine Can you take a quick look at this and kick off some tests?

kkonstantine
kkonstantine reviewed May 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

@gharris1727 are we convinced this is the only outlier?
Also, let's get a system test run too from this branch

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented May 7, 2020

ok to test

@gharris1727
Refactor PluginUtils test and add more isolation test cases
a8ca3bf 
Signed-off-by: Greg Harris <gregh@confluent.io>
@gharris1727
Copy link
Copy Markdown
Contributor Author

gharris1727 commented May 7, 2020 •
edited
Loading

I've refactored the PluginUtilsTest to include more cases, and discovered one discrepancy for the basic-auth-extension.

Based on the pattern from the other modules under connect/ like transforms, I would assume that all of the classes in that module would be isolated by default. This does not appear to be the case:

  • org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension is correctly loaded in isolation
  • org.apache.kafka.connect.rest.basic.auth.extension.JaasBasicAuthFilter is not loaded in isolation but should be
  • org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule is not loaded in isolation but should be

I've left the two classes which do not meet my expectations commented out in the test. I'm not sure what the intended isolation behavior in this case should be.

@liukrimhrim
Copy link
Copy Markdown

liukrimhrim commented May 10, 2020

I've refactored the PluginUtilsTest to include more cases, and discovered one discrepancy for the basic-auth-extension.

Based on the pattern from the other modules under connect/ like transforms, I would assume that all of the classes in that module would be isolated by default. This does not appear to be the case:

  • org.apache.kafka.connect.rest.basic.auth.extension.BasicAuthSecurityRestExtension is correctly loaded in isolation
  • org.apache.kafka.connect.rest.basic.auth.extension.JaasBasicAuthFilter is not loaded in isolation but should be
  • org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule is not loaded in isolation but should be

I've left the two classes which do not meet my expectations commented out in the test. I'm not sure what the intended isolation behavior in this case should be.

Do we have a principle to deduce which class should be loaded in isolation, and which should not? Or is it completely a guess based on experience (if a class is a dep only for plugins, it will be fine to load in isolation)?

@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented May 22, 2020

ok to test

kkonstantine
kkonstantine reviewed Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

The fix makes sense to add, in principle.

Should we explicitly exclude ConnectorClientConfigRequest\\$ClientType too? I noticed it might be loaded first, before ConnectorClientConfigRequest.

Also, please resolve the conflict.

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginUtilsTest.java Outdated
Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginUtilsTest.java Outdated
Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginUtilsTest.java
@gharris1727
Copy link
Copy Markdown
Contributor Author

gharris1727 commented Jun 2, 2020

@kkonstantine I'd appreciate another pass when you're available, thanks!

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Jun 9, 2020

retest this please

kkonstantine
kkonstantine approved these changes Jun 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the fix and the tests @gharris1727

kkonstantine
kkonstantine approved these changes Jun 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the fix and the tests @gharris1727

@kkonstantine kkonstantine merged commit 7d8e62b into apache:trunk Jun 10, 2020
kkonstantine pushed a commit that referenced this pull request Jun 10, 2020
@gharris1727 @kkonstantine
KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading i…
bde48f3 
…solation (#8630)

This fix excludes `ConnectorClientConfigRequest` and its inner class from class loading isolation in a similar way that KAFKA-8415 excluded `ConnectorClientConfigOverridePolicy`.

Reviewer: Konstantine Karantasis <konstantine@confluent.io>
kkonstantine pushed a commit that referenced this pull request Jun 10, 2020
@gharris1727 @kkonstantine
KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading i…
0cde74e 
…solation (#8630)

This fix excludes `ConnectorClientConfigRequest` and its inner class from class loading isolation in a similar way that KAFKA-8415 excluded `ConnectorClientConfigOverridePolicy`.

Reviewer: Konstantine Karantasis <konstantine@confluent.io>
kkonstantine pushed a commit that referenced this pull request Jun 10, 2020
@gharris1727 @kkonstantine
KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading i…
817ed01 
…solation (#8630)

This fix excludes `ConnectorClientConfigRequest` and its inner class from class loading isolation in a similar way that KAFKA-8415 excluded `ConnectorClientConfigOverridePolicy`.

Reviewer: Konstantine Karantasis <konstantine@confluent.io>
kkonstantine pushed a commit that referenced this pull request Jun 10, 2020
@gharris1727 @kkonstantine
KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading i…
a342103 
…solation (#8630)

This fix excludes `ConnectorClientConfigRequest` and its inner class from class loading isolation in a similar way that KAFKA-8415 excluded `ConnectorClientConfigOverridePolicy`.

Reviewer: Konstantine Karantasis <konstantine@confluent.io>
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 13, 2020
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
49b7b7f 
* 'trunk' of github.com:apache/kafka: (42 commits)
  HOTFIX: Fix compile error in TopicAdminTest (apache#8866)
  KAFKA-10144: clean up corrupted standby tasks before attempting a commit (apache#8849)
  KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener (apache#8857)
  KAFKA-9432: automated protocol for DescribeConfigs (apache#8312)
  KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764)
  KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680)
  KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787)
  KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818)
  KAFKA-9374: Make connector interactions asynchronous (apache#8069)
  MINOR: reduce sizeInBytes for percentiles metrics (apache#8835)
  KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829)
  KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828)
  KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455)
  KAFKA-7833: Add missing test (apache#8847)
  KAFKA-9066: Retain metrics for failed tasks (apache#8502)
  KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453)
  KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663)
  KAFKA-9441: remove prepareClose() to simplify task management (apache#8833)
  KAFKA-7833: Add Global/StateStore name conflict check (apache#8825)
  KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630)
  ...
ijuma added a commit to ijuma/kafka that referenced this pull request Nov 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
1869826 
…t-for-generated-requests

* apache-github/trunk: (248 commits)
  KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764)
  KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680)
  KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787)
  KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818)
  KAFKA-9374: Make connector interactions asynchronous (apache#8069)
  MINOR: reduce sizeInBytes for percentiles metrics (apache#8835)
  KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829)
  KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828)
  KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455)
  KAFKA-7833: Add missing test (apache#8847)
  KAFKA-9066: Retain metrics for failed tasks (apache#8502)
  KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453)
  KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663)
  KAFKA-9441: remove prepareClose() to simplify task management (apache#8833)
  KAFKA-7833: Add Global/StateStore name conflict check (apache#8825)
  KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630)
  KAFKA-9991: Fix flaky unit tests (apache#8843)
  KAFKA-10014; Always try to close all channels in Selector#close (apache#8685)
  KAFKA-10079: improve thread-level stickiness (apache#8775)
  MINOR: Print all removed dynamic members during join complete (apache#8816)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkonstantine kkonstantine kkonstantine approved these changes

Assignees

No one assigned

Labels

connect

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gharris1727 @kkonstantine @liukrimhrim @rhauch