You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2025/01/31 19:57:30 UTC
[PR] Gets the correct filesystem when writing an rfile [accumulo]
keith-turner opened a new pull request, #5296:
URL: https://github.com/apache/accumulo/pull/5296
Modified the rfile client code to get the filesystem based on the path being written or read. This covers the case of having multiple filesystems defined in hadoop config.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Gets the correct filesystem when writing an rfile [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #5296:
URL: https://github.com/apache/accumulo/pull/5296
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Gets the correct filesystem when writing an rfile [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org