Skip to content

KAFKA-15203 Use Classgraph since org.reflections is no longer under maintainence#16604

Merged
gharris1727 merged 15 commits intoapache:trunkfrom
PARADOXST:use_classgraph
Aug 19, 2024
Merged

KAFKA-15203 Use Classgraph since org.reflections is no longer under maintainence#16604
gharris1727 merged 15 commits intoapache:trunkfrom
PARADOXST:use_classgraph

Conversation

@PARADOXST
Copy link
Copy Markdown
Contributor

@PARADOXST PARADOXST commented Jul 16, 2024

This is a continuity of #9048 that was paused ~4yrs ago.

https://github.com/ronmamo/reflections is no longer under maintenance and we should switch to
https://github.com/classgraph/classgraph that is more actively maintained and developped.

Committer Checklist (excluded from commit message)

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

@gharris1727
Copy link
Copy Markdown
Contributor

Relevant ticket: https://issues.apache.org/jira/browse/KAFKA-15203

@PARADOXST PARADOXST changed the title Use Classgraph since org.reflections is no longer under maintainence KAFKA-15203 Use Classgraph since org.reflections is no longer under maintainence Jul 26, 2024
@PARADOXST PARADOXST marked this pull request as ready for review July 26, 2024 21:23
@PARADOXST
Copy link
Copy Markdown
Contributor Author

Special thanks to @cushon for helping me fix the problems with the switch!

@PARADOXST
Copy link
Copy Markdown
Contributor Author

PARADOXST commented Jul 29, 2024

CI results: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-16604/7/testReport/org.apache.kafka.connect.runtime/

Test failures in other modules seems irrelevant.

Copy link
Copy Markdown
Contributor

@gharris1727 gharris1727 left a comment

Choose a reason for hiding this comment

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

Thanks @PARADOXST for the PR!

I was under the impression that we were going to be stuck with reflections for a long time, so this is nice to see.

Comment thread gradle/dependencies.gradle Outdated
@gharris1727
Copy link
Copy Markdown
Contributor

I verified that this can discover all of the plugins that the reflections library could find, so this can be a drop-in replacement.

I did notice that it appears much slower than reflections, by about 3x in my environment with 250 plugins. While this isn't a dealbreaker (service_load is already recommended for performance reasons) perhaps there are still some easy wins for optimization. @PARADOXST it's up to you if you're interested in doing an optimization pass, or we could just leave it as-is.

Copy link
Copy Markdown
Contributor Author

@PARADOXST PARADOXST left a comment

Choose a reason for hiding this comment

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

Regarding #16604 (comment): I have replaced enableAllInfo() with enableClassInfo() which should be of some improvements. I am happy to look at further perf improvement as a follow-up.

@PARADOXST
Copy link
Copy Markdown
Contributor Author

@gharris1727 Hi Greg, can you take another look?

Copy link
Copy Markdown
Contributor

@gharris1727 gharris1727 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 @PARADOXST !

I'm still seeing a performance degradation with this swap-out, but as I mentioned earlier I don't think it's a blocker.

@gharris1727
Copy link
Copy Markdown
Contributor

Test failures appear unrelated.

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.

3 participants