ayushtkn commented on code in PR #5990:
URL: https://github.com/apache/hive/pull/5990#discussion_r2265157818
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestCopyUtils.java:
##########
@@ -244,4 +261,91 @@ public void testParallelCopySuccess() throws Exception {
Mockito.times(1)).invokeAll(callableCapture.capture());
}
}
+
+ @Test
+ public void testCopyFilesBetweenFSWithDestDirNotExistFailure() throws
IOException {
+ Path srcPath1 = new Path(basePath, "file1.txt");
+ Path srcPath2 = new Path(basePath, "file2.txt");
+ Path dstPath = new Path(basePath, "copyDst");
+
+ try {
+ // Create source files
+ fileSystem.create(srcPath1).write("Content of file1".getBytes());
+ fileSystem.create(srcPath2).write("Content of file2".getBytes());
+
+ // Prepare source paths array
+ Path[] srcPaths = {srcPath1, srcPath2};
+
+
+ CopyUtils copyUtils = new CopyUtils("hive", hiveConf, fileSystem);
+ DataCopyStatistics copyStatistics = new DataCopyStatistics();
+ IOException thrown =
+ Assert.assertThrows(IOException.class, () -> {
+ copyUtils.copyFilesBetweenFS(fileSystem, srcPaths, fileSystem,
dstPath, false, true, copyStatistics);
+ });
+ // this is supposed to come out of retryable function immediately
without waiting as FileNotFound is not auto-recoverable error
+ Assert.assertEquals(IOException.class, thrown.getCause().getClass());
+ Assert.assertEquals(FileNotFoundException.class,
thrown.getCause().getCause().getClass());
+ Assert.assertEquals("'/tmp/copyDst': specified destination directory
does not exist", thrown.getCause().getMessage());
+
+ } finally {
+ // Clean up
+ fileSystem.delete(srcPath1, false);
+ fileSystem.delete(srcPath2, false);
+ fileSystem.delete(dstPath, true);
+ }
+ }
+
+ @Test
+ public void testCopyFilesBetweenFSWithSourceFileGettingDeletedFailure()
throws IOException {
Review Comment:
I don't think this test is required, it ain't testing the change which we
did, the above test is enough & moreover the catch block below shows like this
might not works sometime, so better to drop it
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestCopyUtils.java:
##########
@@ -244,4 +261,91 @@ public void testParallelCopySuccess() throws Exception {
Mockito.times(1)).invokeAll(callableCapture.capture());
}
}
+
+ @Test
+ public void testCopyFilesBetweenFSWithDestDirNotExistFailure() throws
IOException {
+ Path srcPath1 = new Path(basePath, "file1.txt");
+ Path srcPath2 = new Path(basePath, "file2.txt");
+ Path dstPath = new Path(basePath, "copyDst");
+
+ try {
+ // Create source files
+ fileSystem.create(srcPath1).write("Content of file1".getBytes());
+ fileSystem.create(srcPath2).write("Content of file2".getBytes());
+
+ // Prepare source paths array
+ Path[] srcPaths = {srcPath1, srcPath2};
+
+
+ CopyUtils copyUtils = new CopyUtils("hive", hiveConf, fileSystem);
+ DataCopyStatistics copyStatistics = new DataCopyStatistics();
+ IOException thrown =
+ Assert.assertThrows(IOException.class, () -> {
+ copyUtils.copyFilesBetweenFS(fileSystem, srcPaths, fileSystem,
dstPath, false, true, copyStatistics);
+ });
+ // this is supposed to come out of retryable function immediately
without waiting as FileNotFound is not auto-recoverable error
+ Assert.assertEquals(IOException.class, thrown.getCause().getClass());
+ Assert.assertEquals(FileNotFoundException.class,
thrown.getCause().getCause().getClass());
+ Assert.assertEquals("'/tmp/copyDst': specified destination directory
does not exist", thrown.getCause().getMessage());
Review Comment:
assertThrows throws as an option to assert the exception message as well,
check if you can assert this as part of that only
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestCopyUtils.java:
##########
@@ -244,4 +261,91 @@ public void testParallelCopySuccess() throws Exception {
Mockito.times(1)).invokeAll(callableCapture.capture());
}
}
+
+ @Test
+ public void testCopyFilesBetweenFSWithDestDirNotExistFailure() throws
IOException {
+ Path srcPath1 = new Path(basePath, "file1.txt");
+ Path srcPath2 = new Path(basePath, "file2.txt");
+ Path dstPath = new Path(basePath, "copyDst");
+
+ try {
+ // Create source files
+ fileSystem.create(srcPath1).write("Content of file1".getBytes());
+ fileSystem.create(srcPath2).write("Content of file2".getBytes());
+
+ // Prepare source paths array
+ Path[] srcPaths = {srcPath1, srcPath2};
+
+
+ CopyUtils copyUtils = new CopyUtils("hive", hiveConf, fileSystem);
+ DataCopyStatistics copyStatistics = new DataCopyStatistics();
+ IOException thrown =
+ Assert.assertThrows(IOException.class, () -> {
+ copyUtils.copyFilesBetweenFS(fileSystem, srcPaths, fileSystem,
dstPath, false, true, copyStatistics);
+ });
+ // this is supposed to come out of retryable function immediately
without waiting as FileNotFound is not auto-recoverable error
+ Assert.assertEquals(IOException.class, thrown.getCause().getClass());
Review Comment:
If the above assertion passes, can this ever fail?
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -843,6 +843,9 @@ public static boolean copy(FileSystem srcFS, Path[] srcs,
FileSystem dstFS, Path
if (!doIOUtilsCopyBytes(srcFS, srcFS.getFileStatus(src), dstFS, dst,
deleteSource, overwrite, preserveXAttr, conf, copyStatistics)) {
returnVal = false;
}
+ } catch (FileNotFoundException var15) {
+ // Wrapping the FileNotFoundException in a new IOException and
re-throw immediately.
+ throw new IOException("Copy operation failed", var15);
Review Comment:
why do you want to change exception in the scope of this PR. FNF is already
sorted & not retried right?
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestCopyUtils.java:
##########
@@ -244,4 +261,91 @@ public void testParallelCopySuccess() throws Exception {
Mockito.times(1)).invokeAll(callableCapture.capture());
}
}
+
+ @Test
+ public void testCopyFilesBetweenFSWithDestDirNotExistFailure() throws
IOException {
+ Path srcPath1 = new Path(basePath, "file1.txt");
+ Path srcPath2 = new Path(basePath, "file2.txt");
+ Path dstPath = new Path(basePath, "copyDst");
+
+ try {
+ // Create source files
+ fileSystem.create(srcPath1).write("Content of file1".getBytes());
+ fileSystem.create(srcPath2).write("Content of file2".getBytes());
+
+ // Prepare source paths array
+ Path[] srcPaths = {srcPath1, srcPath2};
+
+
+ CopyUtils copyUtils = new CopyUtils("hive", hiveConf, fileSystem);
+ DataCopyStatistics copyStatistics = new DataCopyStatistics();
+ IOException thrown =
+ Assert.assertThrows(IOException.class, () -> {
+ copyUtils.copyFilesBetweenFS(fileSystem, srcPaths, fileSystem,
dstPath, false, true, copyStatistics);
+ });
+ // this is supposed to come out of retryable function immediately
without waiting as FileNotFound is not auto-recoverable error
+ Assert.assertEquals(IOException.class, thrown.getCause().getClass());
+ Assert.assertEquals(FileNotFoundException.class,
thrown.getCause().getCause().getClass());
Review Comment:
does this work
```
Assert.assertEquals(FileNotFoundException.class,
ExceptionUtils.getRootCause(thrown).getClass());
```
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/repl/TestCopyUtils.java:
##########
@@ -244,4 +261,91 @@ public void testParallelCopySuccess() throws Exception {
Mockito.times(1)).invokeAll(callableCapture.capture());
}
}
+
+ @Test
+ public void testCopyFilesBetweenFSWithDestDirNotExistFailure() throws
IOException {
+ Path srcPath1 = new Path(basePath, "file1.txt");
+ Path srcPath2 = new Path(basePath, "file2.txt");
+ Path dstPath = new Path(basePath, "copyDst");
+
+ try {
+ // Create source files
+ fileSystem.create(srcPath1).write("Content of file1".getBytes());
+ fileSystem.create(srcPath2).write("Content of file2".getBytes());
Review Comment:
who closes the file handle here? you just opened a FS stream & wrote
something on it, didn't flush nor closed it
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
