HDFS-16911. Distcp with snapshot diff to support Ozone filesystem. #5364

sadanand48 · 2023-02-07T15:45:58Z

Description of PR

Currently in DistcpSync i.e the step which applies the diff b/w 2 provided snapshots as arguments to the distcp job with -diff option, only DistributedFilesystem and WebHDFS filesystems are supported.
Now that Ozone supports snapshots, the change here is to make DistcpSync also support OzoneFilesystem (ofs).

The change checks if the filesystem suports fs.hasPathCapability for snapshots and any HCFS that supports snapshots can override hasPathCapability and return true if it does.

Jira :https://issues.apache.org/jira/browse/HDFS-16911

How was this patch tested?

Unit tests
Tested by building hadoop code and replacing distcp jars in Ozone via Unit tests in Ozone.
Also tested on a cluster.

steveloughran

all new methods and fields need a javadoc
what new tests do you propose? the simple one is "how to show the dynamic binding is used"

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpConstants.java

… call respective fs impl

steveloughran

Not going to comment on the snapshot code itself.

I'm worried that removing a constant and the ability to set a new sync class will break something.

Do you know if anyone has actually used this feature?

...ect/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotDiffReport.java

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpConstants.java

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java

steveloughran · 2023-03-15T20:41:47Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSync.java

+      // can expect other exceptions as source and target paths
+      // are not created, assert if the exception is not arising
+      // due to the filesystem not supporting snapshots.
+      Assert.assertFalse(e.getMessage().contains("does not support snapshot"));


are you expecting an exception here? if so, intercept() has an overload where you declare text to look for

Here I'm checking that a particular exception i.e exception stating that 'fs doesn't support snapshots' is not thrown, The intercept utility method checks whether the caught exception has a particular text, I think there is no util method to check its negation.

ok.
Use GenericTestUtils.assertExceptionContains(does not support snapshot", e) so on a failure, the unexpected exception message and stack is not lost. whoever has to debug a test failure from an html test report will love you for this

After changing it to throw UnsupportedOperationException it became easier, The failure scenario for this test case is only if it gets UnsupportedException. It can expect other exceptions.

catch (UnsupportedOperationException e) { Assert.fail("Dummy FS supports snapshots," + "did not expect UnsupportedOperationException"); } catch (Exception e) { // can expect other exceptions as source and target paths // are not created. }

sadanand48 · 2023-03-15T21:03:17Z

I'm worried that removing a constant and the ability to set a new sync class will break something.

Thanks @steveloughran for reviewing this PR. This constant is not present in the hadoop code but was added as part of a previous commit of this same PR, I'm no longer using that approach of setting the distcp.sync.class that I proposed earlier , instead just assigning the correct filesystem dynamically at run time did the job for me.

steveloughran · 2023-03-16T12:05:02Z

thanks for the update; nothing to worry about. I was just concerned if some app was using distcp as a library (hive,...) and this would have broken it.

steveloughran · 2023-03-22T13:30:30Z

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java

    }
    try {
      getSnapshotDiffReportMethod(srcFs);
+    } catch (NoSuchMethodException e) {
+      throw new IllegalArgumentException(


move to UnsupportedOperationException

steveloughran · 2023-03-22T13:30:38Z

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java

@@ -161,20 +161,32 @@ private void checkFilesystemSupport(Path sourceDir, Path targetDir,
    if (!srcFs.hasPathCapability(sourceDir,
        CommonPathCapabilities.FS_SNAPSHOTS)) {
      throw new IllegalArgumentException(


UnsupportedOperationException

steveloughran · 2023-03-22T13:31:20Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSync.java

@@ -1305,7 +1305,7 @@ public void testSyncSnapshotDiffWithDummyFileSystem() throws Exception {
      new DistCp(conf, OptionsParser.parse(args)).execute();
    } catch (Exception e) {


ok. catch UnsupportedOperationException after tightening the exception raised

steveloughran · 2023-03-22T13:33:15Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSync.java

+      // can expect other exceptions as source and target paths
+      // are not created, assert if the exception is not arising
+      // due to the filesystem not supporting snapshots.
+      Assert.assertFalse(e.getMessage().contains("does not support snapshot"));


ok.
Use GenericTestUtils.assertExceptionContains(does not support snapshot", e) so on a failure, the unexpected exception message and stack is not lost. whoever has to debug a test failure from an html test report will love you for this

steveloughran · 2023-03-23T20:33:26Z

had F438 oop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSync.java

@@ -1303,12 +1303,13 @@ public void testSyncSnapshotDiffWithDummyFileSystem() throws Exception {
      FileSystem dummyFs = FileSystem.get(URI.create("dummy:///"), conf);
      Assert.assertTrue(dummyFs instanceof DummyFs);
      new DistCp(conf, OptionsParser.parse(args)).execute();
+    } catch (UnsupportedOperationException e) {
+      Assert.fail("Dummy FS supports snapshots," +


if you just rethrow e here, then the caller gets a full stack.

steveloughran · 2023-03-23T20:35:44Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSync.java

+            "dummy:///target" };
+    try {
+      FileSystem dummyFs = FileSystem.get(URI.create("dummy:///"), conf);
+      Assert.assertTrue(dummyFs instanceof DummyFs);


not very informative. I'd prefer an assertJ assertion that the class is an instance of,

assertThat(dummyfs).isInstanceOf()...

umamaheswararao · 2023-03-23T21:42:02Z

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java

+          fs, snapshotDir, fromSnapshot, toSnapshot);
+    } catch (InvocationTargetException e) {
+      throw new IOException(e.getCause());
+    } catch (NoSuchMethodException|IllegalAccessException e) {


NoSuchMethod sanity check already covered looks like before calling this API. IllegalAccess should noyt happen ideally. So runtime should be ok.
Tiny Nit: Message: "Failed to ...."

Thanks @umamaheswararao for the review.

Done. Yes the exception will likely be detected earlier but these are checked exceptions so it requires either to define in catch block or throws.

umamaheswararao · 2023-03-24T17:07:39Z

This patch looks good to me. If @steveloughran agree that his comments addressed, we can proceed to commit. Thanks
Please take care of nit.

umamaheswararao · 2023-04-10T17:21:51Z

If no other comments, i will proceed to merge this. Thanks

…pache#5364)

…pache#5364) (cherry picked from commit 74ddf69)

HDFS-16911.Distcp with snapshot diff to support Ozone filesystem

b074251

steveloughran requested changes 10000 Feb 7, 2023

View reviewed changes