Allow HoodieWrapperFileSystem to wrap other proxy file-system implementations with no getScheme implementation - #793
Merged
Conversation
…ntations with no getScheme implementation
Member
|
@bhasudha can you also please review this |
| this.uri = uri; | ||
| } | ||
| this.fileSystem = FSUtils.getFs(path.toString(), conf); | ||
| // Do not need to explicitly initialize the default filesystem, its done already in the above |
Member
There was a problem hiding this comment.
do we clean up lines 127-129?
Contributor
vinothchandar
approved these changes
Jul 18, 2019
n3nash
self-requested a review
July 18, 2019 04:32
bvaradar
added a commit
to bvaradar/hudi
that referenced
this pull request
Sep 19, 2019
…ntations with no getScheme implementation (apache#793)
4 tasks
voonhous
pushed a commit
that referenced
this pull request
Aug 3, 2026
* fix(fs): stop depending on the optional FileSystem#getScheme() FileSystem#getScheme() is optional in Hadoop: the base implementation throws UnsupportedOperationException, and proxy implementations such as Presto's PrestoS3FileSystem do not override it. Hudi called it unguarded on filesystems it did not implement, so opening a log file on such a filesystem failed with "Not implemented by the PrestoS3FileSystem FileSystem implementation" instead of reading anything (HUDI-4602). Adds HadoopFSUtils#getScheme(FileSystem), which returns fs.getScheme() and falls back to fs.getUri().getScheme() when it is unimplemented. getUri() is abstract, so every implementation supplies it, and its scheme is what getScheme() returns wherever both are present. This is the same conclusion as #793, which stopped HoodieWrapperFileSystem calling getScheme() on the filesystem it wraps. Routes the seven unguarded call sites through it: isGCSFileSystem and isCHDFileSystem (the reported read path), registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath, HoodieRetryWrapperFileSystem#getScheme, WriteMarkersFactory's HDFS gate, and HoodieHadoopStorage#getScheme, which is what the seven HoodieStorage#getScheme callers reach. isGCSFileSystem's comparison is also flipped to put the constant first, matching isCHDFileSystem, so a filesystem whose URI carries no scheme returns false rather than throwing NullPointerException. * test(fs): say which branch of the helper each assertion covers Review nit: the assertion messages did not make clear what had gone wrong. Each now names the filesystem and the branch of the helper it pins - LocalFileSystem overriding getScheme() so the helper returns what it reports, FilterFileSystem not overriding it so the helper falls back to getUri().getScheme(). * fix(fs): fail loudly on an unresolvable scheme, and cover the sites this reroutes Review feedback, all of it well founded. The fallback no longer returns null. InLineFileSystem is the counter-example in this module: getScheme() is "inlinefs" while getUri() is URI.create("inlinefs"), which has no colon and so no scheme, so the two are not interchangeable and the javadoc claim that they agree was simply wrong. A null surfaced far from the cause as "does not support scheme null" or "Unsupported scheme :null" with the UnsupportedOperationException discarded; it now throws with that exception chained. HoodieException rather than HoodieIOException, since the latter only accepts an IOException cause. HoodieHadoopStorage memoizes the scheme. On a filesystem without getScheme() the fallback costs a thrown-and-caught exception, and this is called once per log block via StorageSchemes.isWriteTransactional and three times per immutable-file write via needCreateTempFile. A lazy field keeps all five constructors untouched. Test coverage for what this actually reroutes, none of which any test reached: - registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath (the write path) and HoodieHadoopStorage#getScheme, via a LocalFileSystem subclass whose getScheme() throws, registered as fs.file.impl so it is reached through FileSystem.get. - isGCSFileSystem and isCHDFileSystem, which become reachable for proxy filesystems for the first time here and select different stream wrappers: a scheme-less filesystem reporting gs:// now yields SchemeAwareFSDataInputStream and ofs:// yields BoundedFsDataInputStream. - the new unresolvable-scheme failure. TestFSUtilsWithRetryWrapperEnable#testGetSchema has been inert since HUDI-5286 added it: it asserted on HoodieWrapperFileSystem#getScheme, which is uri.getScheme() and never dispatches into the retry wrapper, and FakeRemoteFileSystem overrode getScheme() to delegate to a real LocalFileSystem so it could not throw. Dropping that override gives the fake the PrestoS3FileSystem shape and the assertion now targets the retry wrapper, so it guards both HUDI-5286 and this change. Verified: it fails with the pre-PR helper. Also drops the try/catch in convertToHoodiePath that only rethrew HoodieIOException unchanged, dead since ef70de2, and the duplicated fixture and redundant nested close in TestHadoopFSUtils.
voonhous
pushed a commit
that referenced
this pull request
Aug 6, 2026
* fix(fs): stop depending on the optional FileSystem#getScheme() FileSystem#getScheme() is optional in Hadoop: the base implementation throws UnsupportedOperationException, and proxy implementations such as Presto's PrestoS3FileSystem do not override it. Hudi called it unguarded on filesystems it did not implement, so opening a log file on such a filesystem failed with "Not implemented by the PrestoS3FileSystem FileSystem implementation" instead of reading anything (HUDI-4602). Adds HadoopFSUtils#getScheme(FileSystem), which returns fs.getScheme() and falls back to fs.getUri().getScheme() when it is unimplemented. getUri() is abstract, so every implementation supplies it, and its scheme is what getScheme() returns wherever both are present. This is the same conclusion as #793, which stopped HoodieWrapperFileSystem calling getScheme() on the filesystem it wraps. Routes the seven unguarded call sites through it: isGCSFileSystem and isCHDFileSystem (the reported read path), registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath, HoodieRetryWrapperFileSystem#getScheme, WriteMarkersFactory's HDFS gate, and HoodieHadoopStorage#getScheme, which is what the seven HoodieStorage#getScheme callers reach. isGCSFileSystem's comparison is also flipped to put the constant first, matching isCHDFileSystem, so a filesystem whose URI carries no scheme returns false rather than throwing NullPointerException. * test(fs): say which branch of the helper each assertion covers Review nit: the assertion messages did not make clear what had gone wrong. Each now names the filesystem and the branch of the helper it pins - LocalFileSystem overriding getScheme() so the helper returns what it reports, FilterFileSystem not overriding it so the helper falls back to getUri().getScheme(). * fix(fs): fail loudly on an unresolvable scheme, and cover the sites this reroutes Review feedback, all of it well founded. The fallback no longer returns null. InLineFileSystem is the counter-example in this module: getScheme() is "inlinefs" while getUri() is URI.create("inlinefs"), which has no colon and so no scheme, so the two are not interchangeable and the javadoc claim that they agree was simply wrong. A null surfaced far from the cause as "does not support scheme null" or "Unsupported scheme :null" with the UnsupportedOperationException discarded; it now throws with that exception chained. HoodieException rather than HoodieIOException, since the latter only accepts an IOException cause. HoodieHadoopStorage memoizes the scheme. On a filesystem without getScheme() the fallback costs a thrown-and-caught exception, and this is called once per log block via StorageSchemes.isWriteTransactional and three times per immutable-file write via needCreateTempFile. A lazy field keeps all five constructors untouched. Test coverage for what this actually reroutes, none of which any test reached: - registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath (the write path) and HoodieHadoopStorage#getScheme, via a LocalFileSystem subclass whose getScheme() throws, registered as fs.file.impl so it is reached through FileSystem.get. - isGCSFileSystem and isCHDFileSystem, which become reachable for proxy filesystems for the first time here and select different stream wrappers: a scheme-less filesystem reporting gs:// now yields SchemeAwareFSDataInputStream and ofs:// yields BoundedFsDataInputStream. - the new unresolvable-scheme failure. TestFSUtilsWithRetryWrapperEnable#testGetSchema has been inert since HUDI-5286 added it: it asserted on HoodieWrapperFileSystem#getScheme, which is uri.getScheme() and never dispatches into the retry wrapper, and FakeRemoteFileSystem overrode getScheme() to delegate to a real LocalFileSystem so it could not throw. Dropping that override gives the fake the PrestoS3FileSystem shape and the assertion now targets the retry wrapper, so it guards both HUDI-5286 and this change. Verified: it fails with the pre-PR helper. Also drops the try/catch in convertToHoodiePath that only rethrew HoodieIOException unchanged, dead since ef70de2, and the duplicated fixture and redundant nested close in TestHadoopFSUtils. (cherry picked from commit 70a5a4d)
voonhous
pushed a commit
that referenced
this pull request
Aug 7, 2026
* fix(fs): stop depending on the optional FileSystem#getScheme() FileSystem#getScheme() is optional in Hadoop: the base implementation throws UnsupportedOperationException, and proxy implementations such as Presto's PrestoS3FileSystem do not override it. Hudi called it unguarded on filesystems it did not implement, so opening a log file on such a filesystem failed with "Not implemented by the PrestoS3FileSystem FileSystem implementation" instead of reading anything (HUDI-4602). Adds HadoopFSUtils#getScheme(FileSystem), which returns fs.getScheme() and falls back to fs.getUri().getScheme() when it is unimplemented. getUri() is abstract, so every implementation supplies it, and its scheme is what getScheme() returns wherever both are present. This is the same conclusion as #793, which stopped HoodieWrapperFileSystem calling getScheme() on the filesystem it wraps. Routes the seven unguarded call sites through it: isGCSFileSystem and isCHDFileSystem (the reported read path), registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath, HoodieRetryWrapperFileSystem#getScheme, WriteMarkersFactory's HDFS gate, and HoodieHadoopStorage#getScheme, which is what the seven HoodieStorage#getScheme callers reach. isGCSFileSystem's comparison is also flipped to put the constant first, matching isCHDFileSystem, so a filesystem whose URI carries no scheme returns false rather than throwing NullPointerException. * test(fs): say which branch of the helper each assertion covers Review nit: the assertion messages did not make clear what had gone wrong. Each now names the filesystem and the branch of the helper it pins - LocalFileSystem overriding getScheme() so the helper returns what it reports, FilterFileSystem not overriding it so the helper falls back to getUri().getScheme(). * fix(fs): fail loudly on an unresolvable scheme, and cover the sites this reroutes Review feedback, all of it well founded. The fallback no longer returns null. InLineFileSystem is the counter-example in this module: getScheme() is "inlinefs" while getUri() is URI.create("inlinefs"), which has no colon and so no scheme, so the two are not interchangeable and the javadoc claim that they agree was simply wrong. A null surfaced far from the cause as "does not support scheme null" or "Unsupported scheme :null" with the UnsupportedOperationException discarded; it now throws with that exception chained. HoodieException rather than HoodieIOException, since the latter only accepts an IOException cause. HoodieHadoopStorage memoizes the scheme. On a filesystem without getScheme() the fallback costs a thrown-and-caught exception, and this is called once per log block via StorageSchemes.isWriteTransactional and three times per immutable-file write via needCreateTempFile. A lazy field keeps all five constructors untouched. Test coverage for what this actually reroutes, none of which any test reached: - registerFileSystem, HoodieWrapperFileSystem#convertToHoodiePath (the write path) and HoodieHadoopStorage#getScheme, via a LocalFileSystem subclass whose getScheme() throws, registered as fs.file.impl so it is reached through FileSystem.get. - isGCSFileSystem and isCHDFileSystem, which become reachable for proxy filesystems for the first time here and select different stream wrappers: a scheme-less filesystem reporting gs:// now yields SchemeAwareFSDataInputStream and ofs:// yields BoundedFsDataInputStream. - the new unresolvable-scheme failure. TestFSUtilsWithRetryWrapperEnable#testGetSchema has been inert since HUDI-5286 added it: it asserted on HoodieWrapperFileSystem#getScheme, which is uri.getScheme() and never dispatches into the retry wrapper, and FakeRemoteFileSystem overrode getScheme() to delegate to a real LocalFileSystem so it could not throw. Dropping that override gives the fake the PrestoS3FileSystem shape and the assertion now targets the retry wrapper, so it guards both HUDI-5286 and this change. Verified: it fails with the pre-PR helper. Also drops the try/catch in convertToHoodiePath that only rethrew HoodieIOException unchanged, dead since ef70de2, and the duplicated fixture and redundant nested close in TestHadoopFSUtils. (cherry picked from commit 70a5a4d)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue : 786 : HoodieWrapperFileSystem not working with presto