Skip to content

[HUDI-736] Simplify ReflectionUtils#getTopLevelClassesInClasspath - #19784

Open
LeonxLJX wants to merge 3 commits into
apache:masterfrom
LeonxLJX:simplify-getTopLevelClasses
Open

[HUDI-736] Simplify ReflectionUtils#getTopLevelClassesInClasspath#19784
LeonxLJX wants to merge 3 commits into
apache:masterfrom
LeonxLJX:simplify-getTopLevelClasses

Conversation

@LeonxLJX

@LeonxLJX LeonxLJX commented Aug 28, 2026

Copy link
Copy Markdown

Describe the issue this Pull Request addresses

[HUDI-736] ReflectionUtils#getTopLevelClassesInClasspath is written as a verbose imperative loop and contains a latent bug: if ClassLoader#getResources throws an IOException, resources stays null and the subsequent Objects.requireNonNull(resources) throws an NPE instead of degrading gracefully.

Summary and Changelog

  • Rewrite the method as a stream pipeline over the package resources (Collections.list + map/filter/flatMap).
  • Return an empty stream on IOException instead of crashing with an NPE.
  • Extract a small private helper toDirectory to map a resource URL to a File, logging and skipping malformed URLs as before.

Impact

Minor refactor of a utility method used during classpath scanning. Behavior is unchanged for the normal path.

Risk Level

Low - utility method refactor, no behavioral change to public APIs.

Documentation Update

No documentation changes needed.

Contributor's checklist

  • Read through the contributing guide
  • Commit message references the issue
  • Tests added for new functionality
  • Documentation updated where applicable

Replace the imperative loop with a stream pipeline over the package
resources and fix a latent NPE: if ClassLoader#getResources throws an
IOException, the previous code left 'resources' null and then called
Objects.requireNonNull on it. The simplified version returns an empty
stream on error instead.

@utafrali utafrali left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The refactor is a clear improvement: it removes the NPE footgun caused by the null-initialized resources variable and the resulting Objects.requireNonNull crash on IOException. The stream pipeline is more readable. Two issues remain: toDirectory should also catch IllegalArgumentException from new File(URI) (triggered by jar: scheme URLs in all 14 bundle Main callers), and the new IOException recovery path has no test.

Comment thread hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java Outdated
@voonhous

Copy link
Copy Markdown
Member

Please make sure that you follow the PR-description and title template, else CI will fail. Please refer to: https://hudi.apache.org/contribute/how-to-contribute/#opening-pull-requests

Please also mention in the PR body that this closes: #14554

@voonhous voonhous linked an issue Aug 28, 2026 that may be closed by this pull request
@codecov-commenter

codecov-commenter commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.01%. Comparing base (7b377a5) to head (ee98fce).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/apache/hudi/common/util/ReflectionUtils.java 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19784      +/-   ##
============================================
- Coverage     78.15%   78.01%   -0.14%     
- Complexity    33691    34008     +317     
============================================
  Files          2540     2541       +1     
  Lines        141413   142930    +1517     
  Branches      17123    17226     +103     
============================================
+ Hits         110516   111509     +993     
- Misses        23195    23690     +495     
- Partials       7702     7731      +29     
Components Coverage Δ
hudi-common 83.54% <ø> (-0.03%) ⬇️
hudi-client 82.35% <ø> (-0.81%) ⬇️
hudi-flink 85.66% <ø> (ø)
hudi-spark-datasource 72.62% <ø> (-0.08%) ⬇️
hudi-utilities 74.51% <ø> (-0.02%) ⬇️
hudi-cli 15.06% <ø> (ø)
hudi-hadoop 70.11% <ø> (ø)
hudi-sync 75.54% <ø> (-0.03%) ⬇️
hudi-io 81.12% <88.88%> (+1.26%) ⬆️
hudi-timeline-service 83.44% <ø> (ø)
hudi-cloud 65.81% <ø> (ø)
hudi-kafka-connect 53.20% <ø> (ø)
Flag Coverage Δ
common-and-other-modules 51.46% <88.88%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...a/org/apache/hudi/common/util/ReflectionUtils.java 75.00% <88.88%> (+34.70%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…Exception

The classpath resource URL may not always be convertible to a File (e.g.
non-hierarchical or jar-internal schemes), so toDirectory now also catches
IllegalArgumentException instead of propagating it. Adds a unit test
covering getTopLevelClassesInClasspath.
@github-actions github-actions Bot added the size:S PR with lines of changes in (10, 100] label Aug 29, 2026
Removed duplicate imports for List and Collectors.
@hudi-bot

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

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

Labels

size:S PR with lines of changes in (10, 100]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify ReflectionUtils#getTopLevelClasses

5 participants