voonhous commented on PR #19470:
URL: https://github.com/apache/hudi/pull/19470#issuecomment-5169804843
Went through the head commit and every inline thread is addressed, so I have
resolved them. Thanks for the A/B on each one, that made them quick to check.
Three things left, all in the PR description rather than the code:
1. This bullet in Summary and Changelog is now the false claim I asked you
to delete from the javadoc:
> `getUri()` is abstract, so every implementation supplies it, and its
scheme is what `getScheme()` returns wherever both are present.
`InLineFileSystem` is the counter-example, and the helper now throws
rather than returning null, so the bullet should say what the code says:
`getScheme()` first, URI as a fallback, and a hard failure when neither yields
a scheme.
2. The two sentences from my earlier review are still missing:
- #793 fixed only the *instance* methods on `HoodieWrapperFileSystem`.
The static `convertToHoodiePath(StoragePath, Configuration)` kept calling
`getFs(...).getScheme()`, and that is the line this PR finally fixes. Worth
saying, because it explains why the precedent you cite did not already cover
this.
- For a scheme outside the `StorageSchemes` enum the failure moves rather
than disappears: `HoodieLogFileReader:258` and `RollbackHelperV1:190` throw
`IllegalArgumentException: Unsupported scheme` instead of the UOE. Not a
regression, and `s3` is in the enum so the reported case works end to end, but
a reader should not have to derive that.
3. Verification still lists the two tests from the first round. There are
five in `TestHadoopFSUtils` now, plus the retargeted
`TestFSUtilsWithRetryWrapperEnable#testGetSchema`, and the last three are the
ones carrying the weight.
No code changes needed from my side after that.
--
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]
