deepakpanda93 opened a new pull request, #19624:
URL: https://github.com/apache/hudi/pull/19624
### Describe the issue this Pull Request addresses
Closes #14554.
The issue asks for `ReflectionUtils#getTopLevelClassesInClasspath` to be
simplified. Reading it
closely, the method is also broken for the only way it is ever invoked.
Every classpath entry for the package is turned into a `File`:
```java
directories.add(new File(resource.toURI()));
```
A package inside a jar arrives as a `jar:` URL, for example
`jar:file:/path/to/bundle.jar!/org/apache/hudi/hive/bundle`. That URI is not
hierarchical, so `File`
rejects it. Every caller of this method is a bundle `Main` class, which runs
from inside a shaded
jar, so all fourteen of them fail:
```
$ java -cp
packaging/hudi-hive-sync-bundle/target/hudi-hive-sync-bundle-1.3.0-SNAPSHOT.jar
\
org.apache.hudi.hive.bundle.Main
Exception in thread "main" java.lang.IllegalArgumentException: URI is not
hierarchical
at java.base/java.io.File.<init>(File.java:420)
at
org.apache.hudi.common.util.ReflectionUtils.getTopLevelClassesInClasspath(ReflectionUtils.java:145)
at org.apache.hudi.hive.bundle.Main.main(Main.java:34)
```
Those `Main` classes exist to give the packaging modules a source file, as
their javadoc says, so
nothing runs them and the breakage has gone unnoticed. It is a total failure
rather than a latent
one, though, and it is the behaviour anyone gets who runs one.
Two further paths through the method throw rather than report nothing found:
- `getResources` throwing `IOException` is logged, and then
`Objects.requireNonNull(resources)`
turns it into an `NPE` on the next line. The catch makes the failure worse
than no catch at all.
- `clazz.getPackage()` is null for arrays and primitives, and is
dereferenced immediately.
### Summary and Changelog
- Read jar entries through `JarURLConnection` rather than converting the URL
to a `File`. The
directory branch is kept for exploded classpaths, so both layouts are
supported.
- `getResources` failing now yields an empty stream instead of an `NPE`.
- A class with no package now yields an empty stream instead of an `NPE`.
- The recursive `findClasses` helper is replaced by `Files.walk`, and the
method body is a flat
`getResources` to `flatMap` pipeline, which is the simplification the
issue asked for.
- Class names are collected before the jar or the directory walk is closed,
since the returned
stream outlives the method.
`connection.setUseCaches(false)` is set before the `JarFile` is closed. Jar
connections are cached
by default, and closing a cached `JarFile` closes it for every other reader
of the same jar in the
process.
Behaviour that is deliberately unchanged: subpackages are still included, as
the javadoc states, and
the returned names are still fully qualified class names.
### Impact
Bundle `Main` classes run instead of throwing. Nothing else calls this
method, so there is no other
runtime impact.
Callers who previously received an `NPE` from a class with no package, or
from a failed
`getResources`, now receive an empty stream. No caller depended on those
exceptions.
### Risk Level
low
One method and its helpers, in a utility with no production callers beyond
the packaging `Main`
classes. Covered by five new unit tests, and the end to end result is a
bundle `Main` that used to
fail and now succeeds.
### Documentation Update
None. No configuration or public API change; the method signature and
documented behaviour are
unchanged.
### Verification
**End to end, on the real usage.** The same command against the same bundle,
differing only in the
embedded `ReflectionUtils`:
| | Result |
| --- | --- |
| Before | exit 1, `IllegalArgumentException: URI is not hierarchical` |
| After | exit 0, prints `org.apache.hudi.hive.bundle.Main` |
**Unit tests**, added to `TestReflectionUtils`:
| Test | Covers |
| --- | --- |
| `testGetTopLevelClassesInClasspathFromDirectory` | an exploded directory,
built as a fixture, over the `file` protocol |
| `testGetTopLevelClassesInClasspathFromJar` | a jar, built as a fixture,
over the `jar` protocol |
| `testGetTopLevelClassesInClasspathOnTheRealClasspath` | whichever protocol
the JVM presents |
| `testGetTopLevelClassesInClasspathForClassesWithoutAPackage` | arrays and
primitives |
| `testGetTopLevelClassesInClasspathForPackageNotOnTheClasspath` | no
resource matches the package |
Both protocol tests build their own classpath root and load it through a
parent-less
`URLClassLoader`, then assert the exact expected list: two classes, one
class in a subpackage, and
neither a non-class file nor a class outside the package. Building the
fixtures matters, because
under Maven the modules are on the test classpath as jars, so a test that
scans the real classpath
exercises the jar branch and leaves the directory branch uncovered.
**Negative control.** Against the current implementation, exactly three of
the new tests fail, for
the reasons claimed: `FromJar` and `OnTheRealClasspath` with
`IllegalArgumentException: URI is not
hierarchical`, and `ForClassesWithoutAPackage` with
`NullPointerException: Cannot invoke "java.lang.Package.getName()"`.
`FromDirectory` and
`ForPackageNotOnTheClasspath` pass either way, since those paths already
worked.
Full `hudi-io` suite: 125 tests, no failures. `checkstyle` and `apache-rat`
clean on `hudi-io` and
`hudi-common`.
### Related work
#17675 is an open PR against this issue by @VahidRamezaniDB, who found the
jar problem first, after
@yihua asked for a unit test on it. This PR reaches the same conclusion
independently and differs in
two ways: jar entries are read through `JarURLConnection` rather than by
decoding and splitting the
URL string, and the tests build directory and jar fixtures so each protocol
is actually exercised
rather than asserting that a scan of the real classpath returns a non-zero
count.
Happy to close this in favour of that one if a committer prefers; the
reproduction and the before and
after above apply to either.
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Change Logs and Impact were stated clearly
- [x] Adequate tests were added if applicable
- [x] CI passed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
