Skip to content

KAFKA-9712: Catch and handle exception thrown by reflections scanner#8289

Merged
kkonstantine merged 7 commits intoapache:trunkfrom
ncliang:KAFKA-9712
Mar 16, 2020
Merged

KAFKA-9712: Catch and handle exception thrown by reflections scanner#8289
kkonstantine merged 7 commits intoapache:trunkfrom
ncliang:KAFKA-9712

Conversation

@ncliang
Copy link
Copy Markdown
Contributor

@ncliang ncliang commented Mar 12, 2020 •
edited by kkonstantine
Loading

This commit works around a bug in v0.9.12 in upstream reflections library by catching and handling the exception thrown.

The reflections issue is tracked by:
ronmamo/reflections#273

New unit tests were introduced to test the behavior.

Committer Checklist (excluded from commit message)

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

@ncliang
Copy link
Copy Markdown
Contributor Author

ncliang commented Mar 12, 2020

@rhauch @kkonstantine please take a look. The regression was introduced in 2.5, so this fix should also go back as far.

kkonstantine
kkonstantine reviewed Mar 12, 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.

Thanks for the PR @ncliang
In principle, I agree with the approach, but I have a few questions/suggestions.

Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread ...time/src/test/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoaderTest.java Outdated
Comment thread ...time/src/test/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoaderTest.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
ncliang
ncliang commented Mar 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@ncliang ncliang left a comment

Choose a reason for hiding this comment

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

Thanks for review @kkonstantine . I think I have addressed all your comments. Please take another look?

Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread ...time/src/test/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoaderTest.java Outdated
@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 13, 2020

ok to test

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 13, 2020

retest this please

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 16, 2020

retest this please.

kkonstantine
kkonstantine reviewed Mar 16, 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.

@ncliang this looks good overall, but there are a few checkstyle issues that fail the build.

Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
Comment thread .../runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java Outdated
@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 16, 2020

retest this please

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 16, 2020

Rebased to get the recent fix on the unused imports.

@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 16, 2020

retest this please

kkonstantine
kkonstantine approved these changes Mar 16, 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.

Thanks @ncliang
This is a solid fix. LGTM!
I'll merge to trunk and 2.5

@kkonstantine kkonstantine merged commit 569cf99 into apache:trunk Mar 16, 2020
kkonstantine pushed a commit that referenced this pull request Mar 16, 2020
@ncliang @kkonstantine
KAFKA-9712: Catch and handle exception thrown by reflections scanner (#…
e0750e2 
…8289)

This commit works around a bug in version v0.9.12 of the upstream `reflections` library by catching and handling the exception thrown.

The reflections issue is tracked by:
ronmamo/reflections#273

New unit tests were introduced to test the behavior.

* KAFKA-9712: Catch and handle exception thrown by reflections scanner

* Update connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java

Co-Authored-By: Konstantine Karantasis <konstantine@confluent.io>

* Move result initialization back to right before it is used

* Use `java.io.File` in tests

* Fix checkstyle

Co-authored-by: Konstantine Karantasis <konstantine@confluent.io>

Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Mar 16, 2020

Merged to trunk and cherry-picked on 2.5

ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into ccs-master
6f7084a 
* apache-github/trunk: (39 commits)
  MINOR: cleanup and add tests to StateDirectoryTest (apache#8304)
  HOTFIX: StateDirectoryTest should use Set instead of List (apache#8305)
  MINOR: Fix build and JavaDoc warnings (apache#8291)
  MINOR: Fix kafka.server.RequestQuotaTest missing new ApiKeys. (apache#8302)
  KAFKA-9712: Catch and handle exception thrown by reflections scanner (apache#8289)
  KAFKA-9670; Reduce allocations in Metadata Response preparation (apache#8236)
  MINOR: fix Scala 2.13 build error introduced in apache#8083 (apache#8301)
  MINOR: enforce non-negative invariant for checkpointed offsets (apache#8297)
  MINOR: comment apikey types in generated switch (apache#8201)
  MINOR: Fix typo in CreateTopicsResponse.json (apache#8300)
  KIP-546: Implement describeClientQuotas and alterClientQuotas. (apache#8083)
  KAFKA-6647: Do note delete the lock file while holding the lock (apache#8267)
  KAFKA-9677: Fix consumer fetch with small consume bandwidth quotas (apache#8290)
  KAFKA-9533: Fix JavaDocs of KStream.transformValues (apache#8298)
  MINOR: reuse pseudo-topic in FKJoin (apache#8296)
  KAFKA-6145: Pt 2. Include offset sums in subscription (apache#8246)
  KAFKA-9714; Eliminate unused reference to IBP in `TransactionStateManager` (apache#8293)
  KAFKA-9718; Don't log passwords for AlterConfigs in request logs (apache#8294)
  KAFKA-8768: DeleteRecords request/response automated protocol (apache#7957)
  KAFKA-9685: Solve Set concatenation perf issue in AclAuthorizer
  ...
andrewegel pushed a commit to andrewegel/kafka that referenced this pull request Apr 28, 2021
@ncliang @andrewegel
KAFKA-9712: Catch and handle exception thrown by reflections scanner (a…
3909574 
…pache#8289)

This commit works around a bug in version v0.9.12 of the upstream `reflections` library by catching and handling the exception thrown.

The reflections issue is tracked by:
ronmamo/reflections#273

New unit tests were introduced to test the behavior.

* KAFKA-9712: Catch and handle exception thrown by reflections scanner

* Update connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java

Co-Authored-By: Konstantine Karantasis <konstantine@confluent.io>

* Move result initialization back to right before it is used

* Use `java.io.File` in tests

* Fix checkstyle

Co-authored-by: Konstantine Karantasis <konstantine@confluent.io>

Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
andrewegel pushed a commit to confluentinc/kafka that referenced this pull request May 25, 2021
@ncliang @andrewegel
KAFKA-9712: Catch and handle exception thrown by reflections scanner (a…
d861f1e 
…pache#8289)

This commit works around a bug in version v0.9.12 of the upstream `reflections` library by catching and handling the exception thrown.

The reflections issue is tracked by:
ronmamo/reflections#273

New unit tests were introduced to test the behavior.

* KAFKA-9712: Catch and handle exception thrown by reflections scanner

* Update connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java

Co-Authored-By: Konstantine Karantasis <konstantine@confluent.io>

* Move result initialization back to right before it is used

* Use `java.io.File` in tests

* Fix checkstyle

Co-authored-by: Konstantine Karantasis <konstantine@confluent.io>

Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
andrewegel pushed a commit to confluentinc/kafka that referenced this pull request May 25, 2021
@ncliang @andrewegel
KAFKA-9712: Catch and handle exception thrown by reflections scanner (a…
8b751fb 
…pache#8289)

This commit works around a bug in version v0.9.12 of the upstream `reflections` library by catching and handling the exception thrown.

The reflections issue is tracked by:
ronmamo/reflections#273

New unit tests were introduced to test the behavior.

* KAFKA-9712: Catch and handle exception thrown by reflections scanner

* Update connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java

Co-Authored-By: Konstantine Karantasis <konstantine@confluent.io>

* Move result initialization back to right before it is used

* Use `java.io.File` in tests

* Fix checkstyle

Co-authored-by: Konstantine Karantasis <konstantine@confluent.io>

Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 left review comments

@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.

3 participants

@ncliang @kkonstantine @chia7712