rangareddy commented on code in PR #19470:
URL: https://github.com/apache/hudi/pull/19470#discussion_r3704438904
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestHadoopFSUtils.java:
##########
@@ -22,22 +22,80 @@
import org.apache.hudi.storage.StoragePath;
import org.apache.hudi.storage.StoragePathInfo;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
import org.apache.hadoop.fs.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
import static
org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopFileStatus;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopPath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePathInfo;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
/**
* Tests {@link HadoopFSUtils}
*/
public class TestHadoopFSUtils {
+ /**
+ * HUDI-4602: {@link FileSystem#getScheme()} is optional in Hadoop -- the
base implementation throws
+ * {@link UnsupportedOperationException} -- and proxy implementations such
as Presto's
+ * {@code PrestoS3FileSystem} do not override it. Opening a log file went
straight through
+ * {@code isGCSFileSystem}, so a MOR {@code _rt} query on Presto failed with
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
rather than reading anything.
+ *
+ * <p>{@link FilterFileSystem} has the same shape: it leaves {@code
getScheme()} to the throwing base
+ * implementation while overriding {@code getUri()}.
+ */
+ @Test
+ public void testGetFSDataInputStreamWhenGetSchemeIsUnimplemented(@TempDir
File tempDir) throws IOException {
+ File file = new File(tempDir, "log.file");
+ byte[] contents = new byte[] {1, 2, 3, 4};
+ Files.write(file.toPath(), contents);
+ // newInstanceLocal rather than getLocal, so closing this does not evict a
cached FileSystem that
+ // other tests in the same JVM share.
+ try (FileSystem fs = new FilterFileSystem(FileSystem.newInstanceLocal(new
Configuration()))) {
Review Comment:
Extracted, with the premise assertion inside it so it cannot drift between
call sites:
```java
/** A FileSystem with the reported shape: {@code getUri()} works, {@code
getScheme()} throws. */
private static FileSystem newFsWithoutGetScheme(FileSystem delegate) {
FileSystem fs = new FilterFileSystem(delegate);
// The premise of every assertion below: this is the call the read path
used to make unguarded.
assertThrows(UnsupportedOperationException.class, fs::getScheme);
return fs;
}
```
There is a second fixture alongside it, `NoSchemeFileSystem`, for the cases
that need a chosen URI (`gs://`, `ofs://`, scheme-less) rather than the
delegate's.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestHadoopFSUtils.java:
##########
@@ -22,22 +22,80 @@
import org.apache.hudi.storage.StoragePath;
import org.apache.hudi.storage.StoragePathInfo;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
import org.apache.hadoop.fs.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
import static
org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopFileStatus;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopPath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePathInfo;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
/**
* Tests {@link HadoopFSUtils}
*/
public class TestHadoopFSUtils {
+ /**
+ * HUDI-4602: {@link FileSystem#getScheme()} is optional in Hadoop -- the
base implementation throws
+ * {@link UnsupportedOperationException} -- and proxy implementations such
as Presto's
+ * {@code PrestoS3FileSystem} do not override it. Opening a log file went
straight through
+ * {@code isGCSFileSystem}, so a MOR {@code _rt} query on Presto failed with
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
rather than reading anything.
+ *
+ * <p>{@link FilterFileSystem} has the same shape: it leaves {@code
getScheme()} to the throwing base
+ * implementation while overriding {@code getUri()}.
+ */
+ @Test
+ public void testGetFSDataInputStreamWhenGetSchemeIsUnimplemented(@TempDir
File tempDir) throws IOException {
+ File file = new File(tempDir, "log.file");
+ byte[] contents = new byte[] {1, 2, 3, 4};
+ Files.write(file.toPath(), contents);
+ // newInstanceLocal rather than getLocal, so closing this does not evict a
cached FileSystem that
+ // other tests in the same JVM share.
+ try (FileSystem fs = new FilterFileSystem(FileSystem.newInstanceLocal(new
Configuration()))) {
+ // The premise: this is the call the read path used to make unguarded.
+ assertThrows(UnsupportedOperationException.class, fs::getScheme);
+
+ try (FSDataInputStream stream =
+ HadoopFSUtils.getFSDataInputStream(fs, new
StoragePath(file.toURI()), 1024, true)) {
+ byte[] read = new byte[contents.length];
+ stream.readFully(read);
+ assertArrayEquals(contents, read, "The read path should not depend on
the optional getScheme()");
+ }
+ }
+ }
+
+ @Test
+ public void testGetSchemeFallsBackToTheUriWhenUnimplemented() throws
IOException {
+ try (FileSystem localFs = FileSystem.newInstanceLocal(new
Configuration())) {
+ assertEquals("file", HadoopFSUtils.getScheme(localFs),
+ "LocalFileSystem overrides getScheme(), so the helper should return
what it reports "
+ + "rather than falling back to getUri()");
+
+ try (FileSystem noScheme = new FilterFileSystem(localFs)) {
+ assertThrows(UnsupportedOperationException.class, noScheme::getScheme);
+ assertEquals("file", HadoopFSUtils.getScheme(noScheme),
+ "FilterFileSystem does not override getScheme(), so the helper
should fall back to "
+ + "getUri().getScheme()");
+ }
Review Comment:
Applied. The nested block is gone and the comment about why
`newInstanceLocal` is used now stands on its own:
```java
// FilterFileSystem#close closes the delegate, so the wrapper is not given
its own block: it owns
// nothing, and closing it here would close localFs a second time.
FileSystem noScheme = newFsWithoutGetScheme(localFs);
```
Thanks for checking that the double close was harmless rather than just
asserting it — you are right that the block implied ownership that does not
exist.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestHadoopFSUtils.java:
##########
@@ -22,22 +22,80 @@
import org.apache.hudi.storage.StoragePath;
import org.apache.hudi.storage.StoragePathInfo;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
import org.apache.hadoop.fs.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
import static
org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopFileStatus;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopPath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePathInfo;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
/**
* Tests {@link HadoopFSUtils}
*/
public class TestHadoopFSUtils {
+ /**
+ * HUDI-4602: {@link FileSystem#getScheme()} is optional in Hadoop -- the
base implementation throws
+ * {@link UnsupportedOperationException} -- and proxy implementations such
as Presto's
+ * {@code PrestoS3FileSystem} do not override it. Opening a log file went
straight through
+ * {@code isGCSFileSystem}, so a MOR {@code _rt} query on Presto failed with
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
rather than reading anything.
+ *
+ * <p>{@link FilterFileSystem} has the same shape: it leaves {@code
getScheme()} to the throwing base
+ * implementation while overriding {@code getUri()}.
+ */
+ @Test
+ public void testGetFSDataInputStreamWhenGetSchemeIsUnimplemented(@TempDir
File tempDir) throws IOException {
Review Comment:
Switched off `File`, with one compromise: this file also uses Hadoop's
`Path` in `testPathConversion` and `testFileStatusConversion`, so the two
cannot both be imported unqualified. I qualified the nio one at its four use
sites rather than rewrite the pre-existing conversion tests to
`org.apache.hadoop.fs.Path`:
```java
public void testGetFSDataInputStreamWhenGetSchemeIsUnimplemented(@TempDir
java.nio.file.Path tempDir)
```
Verbose, but it keeps the diff to the tests this PR is actually about, and
the `java.io.File` import is gone along with the `file.toPath()` round-trip.
Happy to do the unqualified version and convert the Hadoop `Path` uses if you
would prefer the file consistent.
You were right that this needed `toURI()` → `toUri()` as well.
--
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]
