rangareddy commented on code in PR #19470:
URL: https://github.com/apache/hudi/pull/19470#discussion_r3704437325
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HoodieRetryWrapperFileSystem.java:
##########
@@ -277,7 +277,7 @@ public Configuration getConf() {
@Override
public String getScheme() {
- return fileSystem.getScheme();
+ return HadoopFSUtils.getScheme(fileSystem);
Review Comment:
Applied exactly as suggested, and you are right that it had been a no-op
since it merged. Deleted `FakeRemoteFileSystem#getScheme()` and retargeted the
assertion at the wrapper:
```java
assertThrows(UnsupportedOperationException.class, fakeFs::getScheme);
assertEquals("file", ((HoodieRetryWrapperFileSystem)
fileSystem).getScheme(), ...);
```
`FakeRemoteFileSystem extends FileSystem`, so with the override gone the
base implementation throws and `getUri()` still delegates to the real local
filesystem — the `PrestoS3FileSystem` shape, as you said.
Confirmed it is now a real guard: with the pre-PR helper restored,
`TestFSUtilsWithRetryWrapperEnable` reports `Tests run: 55, Errors: 1` and the
failing one is `testGetSchema`. It could not fail before.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HadoopFSUtils.java:
##########
@@ -294,7 +315,7 @@ private static StorageConfiguration<Configuration>
getStorageConf(Configuration
public static Configuration registerFileSystem(StoragePath file,
Configuration conf) {
Configuration returnConf = new Configuration(conf);
- String scheme = HadoopFSUtils.getFs(file.toString(), conf).getScheme();
+ String scheme = getScheme(HadoopFSUtils.getFs(file.toString(), conf));
Review Comment:
Added, with the fixture registered as `fs.file.impl` so all three go through
the normal `FileSystem.get` path:
```java
Configuration conf = new Configuration();
conf.setClass("fs.file.impl", NoSchemeLocalFileSystem.class,
FileSystem.class);
StoragePath path = new StoragePath(tempDir.toUri());
assertDoesNotThrow(() -> HadoopFSUtils.registerFileSystem(path, conf), ...);
assertDoesNotThrow(() -> HoodieWrapperFileSystem.convertToHoodiePath(path,
conf), ...);
assertEquals("file", new HoodieHadoopStorage(path,
HadoopFSUtils.getStorageConf(conf)).getScheme(), ...);
```
`testCallSitesWorkOnAFileSystemWithoutGetScheme`. With the pre-PR helper
restored it errors on all three, so it reproduces your A/B.
Your point about `convertToHoodiePath` being the write path is what made me
stop treating this as a read-path fix — thanks for spelling out the HUDI-4495
lineage.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HadoopFSUtils.java:
##########
@@ -277,15 +277,36 @@ private static FSDataInputStream
getFSDataInputStreamForGCS(FSDataInputStream fs
* @return true if the inputstream or the wrapped one is of type
GoogleHadoopFSInputStream
*/
public static boolean isGCSFileSystem(FileSystem fs) {
- return fs.getScheme().equals(StorageSchemes.GCS.getScheme());
+ return StorageSchemes.GCS.getScheme().equals(getScheme(fs));
Review Comment:
Added `testSchemeSpecificStreamIsSelectedWithoutGetScheme`, parameterised
over both predicates, using a `FilterFileSystem` subclass that reports a URI of
my choosing while leaving `getScheme()` to the throwing base implementation:
```java
@CsvSource({
"gs://bucket, org.apache.hudi.hadoop.fs.SchemeAwareFSDataInputStream",
"ofs://cluster, org.apache.hudi.hadoop.fs.BoundedFsDataInputStream"
})
```
It passes, which independently confirms your A/B: the fallback-resolved
scheme does select `SchemeAwareFSDataInputStream` and
`BoundedFsDataInputStream`. With the pre-PR helper both cases error instead.
Agreed this is the assertion with the most future value — it is the one that
pins `HadoopFSUtils.getScheme` returning what `fs.getScheme()` would have,
rather than merely not throwing.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HadoopFSUtils.java:
##########
@@ -277,15 +277,36 @@ private static FSDataInputStream
getFSDataInputStreamForGCS(FSDataInputStream fs
* @return true if the inputstream or the wrapped one is of type
GoogleHadoopFSInputStream
*/
public static boolean isGCSFileSystem(FileSystem fs) {
- return fs.getScheme().equals(StorageSchemes.GCS.getScheme());
+ return StorageSchemes.GCS.getScheme().equals(getScheme(fs));
}
/**
* Chdfs will throw {@code IOException} instead of {@code EOFException}. It
will cause error in isBlockCorrupted().
* Wrapped by {@code BoundedFsDataInputStream}, to check whether the desired
offset is out of the file size in advance.
*/
public static boolean isCHDFileSystem(FileSystem fs) {
- return StorageSchemes.CHDFS.getScheme().equals(fs.getScheme());
+ return StorageSchemes.CHDFS.getScheme().equals(getScheme(fs));
+ }
+
+ /**
+ * Resolves the scheme of {@code fs} without depending on {@link
FileSystem#getScheme()}.
+ *
+ * <p>{@code getScheme()} is optional in Hadoop: {@link FileSystem}'s own
implementation throws
+ * {@link UnsupportedOperationException}, and proxy implementations such as
Presto's
+ * {@code PrestoS3FileSystem} do not override it, so calling it unguarded
turns an unrelated read into
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
(HUDI-4602).
+ * {@link FileSystem#getUri()} is abstract, so every implementation supplies
it, and its scheme is what
+ * {@code getScheme()} returns wherever both are present.
+ *
+ * @param fs instance of {@link FileSystem} in use.
+ * @return the scheme of {@code fs}, or null if its URI carries none.
+ */
+ public static String getScheme(FileSystem fs) {
+ try {
+ return fs.getScheme();
+ } catch (UnsupportedOperationException e) {
+ return fs.getUri().getScheme();
+ }
Review Comment:
You are right, and this is the most important comment on the PR — the
javadoc sentence you quote was simply false, and `InLineFileSystem` is the
counter-example sitting in the same module. Verified:
`URI.create("inlinefs").getScheme()` is `null`.
Applied, with one deviation. Your snippet does not compile as written:
`HoodieIOException`'s only cause-taking constructor is
`HoodieIOException(String, IOException)`, so an `UnsupportedOperationException`
cannot be chained through it — and discarding that cause is the thing being
fixed. I used `HoodieException`, which takes a `Throwable`:
```java
} catch (UnsupportedOperationException e) {
String scheme = fs.getUri().getScheme();
if (scheme == null) {
throw new HoodieException("Cannot resolve the scheme of " +
fs.getClass().getName()
+ ": getScheme() is unimplemented and its URI " + fs.getUri() + "
carries no scheme", e);
}
return scheme;
}
```
Say the word if you would rather keep `HoodieIOException` and wrap the cause
in an `IOException` to fit its signature; I preferred not to invent a layer.
Both claims are gone from the javadoc. It now states the opposite — that the
two sources are *not* interchangeable, names `InLineFileSystem` as the reason,
and says why the ordering is load-bearing rather than cosmetic. `@return` is
`never null`, with `@throws`.
`testGetSchemeFailsLoudlyWhenNeitherSourceHasOne` pins it and asserts the
cause is chained. With the null-returning fallback restored it fails with
`Expected org.apache.hudi.exception.HoodieException to be thrown, but nothing
was thrown.`
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HoodieHadoopStorage.java:
##########
@@ -111,7 +111,7 @@ public HoodieStorage newInstance(StoragePath path,
StorageConfiguration<?> stora
@Override
public String getScheme() {
- return fs.getScheme();
+ return HadoopFSUtils.getScheme(fs);
Review Comment:
Memoized. Thank you for measuring it — the per-log-block call through
`isWriteTransactional` is the one that convinced me, since that is squarely
this PR's own target path.
```java
private final Lazy<String> scheme = Lazy.lazily(this::resolveScheme);
@Override
public String getScheme() {
return scheme.get();
}
private String resolveScheme() {
return HadoopFSUtils.getScheme(fs);
}
```
One wrinkle: `Lazy.lazily(() -> HadoopFSUtils.getScheme(fs))` as a field
initializer does not compile — `fs` is a blank final assigned in the
constructors, so javac reports "variable fs might not have been initialized". A
method reference defers the read and still avoids touching all five
constructors, which was your point.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HoodieWrapperFileSystem.java:
##########
@@ -160,7 +160,7 @@ public HoodieWrapperFileSystem(FileSystem fileSystem,
ConsistencyGuard consisten
public static Path convertToHoodiePath(StoragePath file, Configuration conf)
{
try {
- String scheme = HadoopFSUtils.getFs(file.toString(), conf).getScheme();
+ String scheme =
HadoopFSUtils.getScheme(HadoopFSUtils.getFs(file.toString(), conf));
Review Comment:
Dropped — the `catch (HoodieIOException e) { throw e; }` was indeed a no-op:
```java
public static Path convertToHoodiePath(StoragePath file, Configuration conf)
{
String scheme =
HadoopFSUtils.getScheme(HadoopFSUtils.getFs(file.toString(), conf));
return convertPathWithScheme(convertToHadoopPath(file),
getHoodieScheme(scheme));
}
```
Worth noting this method is now covered by
`testCallSitesWorkOnAFileSystemWithoutGetScheme` from your other comment, so
the removal is not unobserved.
--
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]
