ddanielr commented on code in PR #5296:
URL: https://github.com/apache/accumulo/pull/5296#discussion_r1937951794
##########
core/src/test/java/org/apache/accumulo/core/client/rfile/RFileClientTest.java:
##########
@@ -840,4 +842,35 @@ public void testMultipleFilesAndCache() throws Exception {
assertEquals(testData, toMap(scanner));
scanner.close();
}
+
+ @Test
+ public void testFileSystemFromUri() throws Exception {
+ String localFsClass = "LocalFileSystem";
+
+ String fileUri = "hdfs://127.0.0.5:8080/bulk-xyx/file1.rf";
+ // There was a bug in the code where the default hadoop file system was
always used. This test
+ // checks that the hadoop filesystem used it based on the URI and not the
default filesystem. In
+ // this env the default file system is the local hadoop file system.
+ var exception = assertThrows(IOException.class, () ->
RFile.newWriter().to(fileUri).build());
+ // Ensure the DistributedFileSystem was used.
+ assertTrue(Arrays.stream(exception.getStackTrace())
+ .anyMatch(ste ->
ste.getClassName().contains(DistributedFileSystem.class.getName())));
+ assertTrue(Arrays.stream(exception.getStackTrace())
+ .noneMatch(ste -> ste.getClassName().contains(localFsClass)));
+
+ var exception2 = assertThrows(RuntimeException.class, () -> {
+ var scanner = RFile.newScanner().from(fileUri).build();
+ scanner.iterator();
+ });
+ assertTrue(Arrays.stream(exception2.getCause().getStackTrace())
+ .anyMatch(ste ->
ste.getClassName().contains(DistributedFileSystem.class.getName())));
+ assertTrue(Arrays.stream(exception2.getCause().getStackTrace())
+ .noneMatch(ste -> ste.getClassName().contains(localFsClass)));
+
+ // verify the assumptions this test is making about the local filesystem
being the default.
+ var exception3 = assertThrows(IllegalArgumentException.class,
+ () -> FileSystem.get(new Configuration()).open(new Path(fileUri)));
Review Comment:
Check that the exception class is erroring the way we expect.
```suggestion
() -> FileSystem.get(new Configuration()).open(new Path(fileUri)));
assertTrue(exception3.getMessage().contains("Wrong FS: " + fileUri +
", expected: file:///"));
```
##########
core/src/test/java/org/apache/accumulo/core/client/rfile/RFileClientTest.java:
##########
@@ -840,4 +842,35 @@ public void testMultipleFilesAndCache() throws Exception {
assertEquals(testData, toMap(scanner));
scanner.close();
}
+
+ @Test
+ public void testFileSystemFromUri() throws Exception {
+ String localFsClass = "LocalFileSystem";
+
+ String fileUri = "hdfs://127.0.0.5:8080/bulk-xyx/file1.rf";
+ // There was a bug in the code where the default hadoop file system was
always used. This test
+ // checks that the hadoop filesystem used it based on the URI and not the
default filesystem. In
+ // this env the default file system is the local hadoop file system.
+ var exception = assertThrows(IOException.class, () ->
RFile.newWriter().to(fileUri).build());
Review Comment:
Tested this locally and it throws a `ConnectException.class`.
If we wanted to be more specific we could capture the remoteFsHost address
of `127.0.0.5:8080` and check the exception message for it.
```suggestion
var exception = assertThrows(IOException.class, () ->
RFile.newWriter().to(fileUri).build());
var exception =
assertThrows(ConnectException.class, () ->
RFile.newWriter().to(fileUri).build());
assertTrue(exception.getMessage().contains("to " + remoteFsHost
+ " failed on connection exception: java.net.ConnectException:
Connection refused"));
```
##########
core/src/test/java/org/apache/accumulo/core/client/rfile/RFileClientTest.java:
##########
@@ -840,4 +842,35 @@ public void testMultipleFilesAndCache() throws Exception {
assertEquals(testData, toMap(scanner));
scanner.close();
}
+
+ @Test
+ public void testFileSystemFromUri() throws Exception {
+ String localFsClass = "LocalFileSystem";
+
+ String fileUri = "hdfs://127.0.0.5:8080/bulk-xyx/file1.rf";
+ // There was a bug in the code where the default hadoop file system was
always used. This test
+ // checks that the hadoop filesystem used it based on the URI and not the
default filesystem. In
+ // this env the default file system is the local hadoop file system.
+ var exception = assertThrows(IOException.class, () ->
RFile.newWriter().to(fileUri).build());
+ // Ensure the DistributedFileSystem was used.
+ assertTrue(Arrays.stream(exception.getStackTrace())
+ .anyMatch(ste ->
ste.getClassName().contains(DistributedFileSystem.class.getName())));
+ assertTrue(Arrays.stream(exception.getStackTrace())
+ .noneMatch(ste -> ste.getClassName().contains(localFsClass)));
+
+ var exception2 = assertThrows(RuntimeException.class, () -> {
+ var scanner = RFile.newScanner().from(fileUri).build();
+ scanner.iterator();
+ });
Review Comment:
Same specific check here:
```suggestion
});
assertTrue(exception2.getMessage().contains("to " + remoteFsHost
+ " failed on connection exception: java.net.ConnectException:
Connection refused"));
```
##########
core/src/test/java/org/apache/accumulo/core/client/rfile/RFileClientTest.java:
##########
@@ -840,4 +842,35 @@ public void testMultipleFilesAndCache() throws Exception {
assertEquals(testData, toMap(scanner));
scanner.close();
}
+
+ @Test
+ public void testFileSystemFromUri() throws Exception {
+ String localFsClass = "LocalFileSystem";
+
+ String fileUri = "hdfs://127.0.0.5:8080/bulk-xyx/file1.rf";
+ // There was a bug in the code where the default hadoop file system was
always used. This test
+ // checks that the hadoop filesystem used it based on the URI and not the
default filesystem. In
+ // this env the default file system is the local hadoop file system.
+ var exception = assertThrows(IOException.class, () ->
RFile.newWriter().to(fileUri).build());
Review Comment:
You'll need to add `import java.net.ConnectException;` for this suggestion.
I have these changes locally if you want me to push a commit.
--
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]
