deepakpanda93 commented on PR #19624: URL: https://github.com/apache/hudi/pull/19624#issuecomment-5600697821
Thanks @voonhous — this was a genuinely useful pass. Description rewritten, and every inline comment is either applied or answered below. **On the slf4j question: you are right, and I was wrong.** No, slf4j-api was not on the bundle's own classpath — I ran it with slf4j supplied, which is not how anyone runs the bare bundle jar, so the claim was measured under conditions that do not match what I claimed. I checked all the built bundles rather than just the one you named: ``` bundle ReflectionUtils LoggerFactory hudi-cli-bundle_2.12 1 1 hudi-aws-bundle 1 0 hudi-azure-bundle 1 0 hudi-datahub-sync-bundle 1 0 hudi-flink1.20-bundle 1 0 hudi-gcp-bundle 1 0 hudi-hadoop-mr-bundle 1 0 hudi-hive-sync-bundle 1 0 hudi-kafka-connect-bundle 1 0 hudi-presto-bundle 1 0 hudi-spark4.2-bundle_2.13 1 0 hudi-timeline-server-bundle 1 0 hudi-trino-bundle 1 0 hudi-utilities-bundle_2.12 1 0 hudi-utilities-slim-bundle_2.12 1 0 ``` 15 bundles carry `ReflectionUtils`, 1 carries `LoggerFactory`. `ReflectionUtils` is `@Slf4j`, so the generated static `Logger` means `<clinit>` needs that class and `Main` dies before reaching this code in the other 14. The Impact section now states that scope and says plainly that the earlier claim was wrong. The other four description points are all correct and all fixed: - The `getResources` NPE fix and the flat `flatMap` pipeline were #19784's; they are gone from the changelog. "`findClasses` is replaced by `Files.walk`" is gone too — `findClasses` is untouched apart from the `listFiles` guard and the shared constant. - The before/after table and the stack trace quoting `ReflectionUtils.java:145` are gone. The description now leads with the silent no-op, as you suggest, and notes the `IllegalArgumentException` only as the pre-#19784 behaviour. - The test table is gone; the verification section lists what the diff actually adds, names `hudi-common`, and gives the real negative-control outcome for `FromJar` (empty result, not an exception). - "Closes #14554" and the reference to #17675 as open are both removed. Thanks for confirming the code change itself, and for closing #17675 in favour of this one. -- 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]
