HDFS-10397. Distcp should ignore -delete option if -diff option is provided instead of exiting. Contributed by Mingliang Liu.

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

@@ -160,7 +160,6 @@ public class DistCpOptions {
    * @param atomicCommit - boolean switch
    */
   public void setAtomicCommit(boolean atomicCommit) {
-    validate(DistCpOptionSwitch.ATOMIC_COMMIT, atomicCommit);
     this.atomicCommit = atomicCommit;
   }
 
@@ -179,7 +178,6 @@ public class DistCpOptions {
    * @param syncFolder - boolean switch
    */
   public void setSyncFolder(boolean syncFolder) {
-    validate(DistCpOptionSwitch.SYNC_FOLDERS, syncFolder);
     this.syncFolder = syncFolder;
   }
 
@@ -198,7 +196,6 @@ public class DistCpOptions {
    * @param deleteMissing - boolean switch
    */
   public void setDeleteMissing(boolean deleteMissing) {
-    validate(DistCpOptionSwitch.DELETE_MISSING, deleteMissing);
     this.deleteMissing = deleteMissing;
   }
 
@@ -254,7 +251,6 @@ public class DistCpOptions {
    * @param overwrite - boolean switch
    */
   public void setOverwrite(boolean overwrite) {
-    validate(DistCpOptionSwitch.OVERWRITE, overwrite);
     this.overwrite = overwrite;
   }
 
@@ -270,7 +266,6 @@ public class DistCpOptions {
    * update option and CRC is not skipped.
    */
   public void setAppend(boolean append) {
-    validate(DistCpOptionSwitch.APPEND, append);
     this.append = append;
   }
 
@@ -287,7 +282,6 @@ public class DistCpOptions {
   }
 
   public void setUseDiff(boolean useDiff, String fromSnapshot, String toSnapshot) {
-    validate(DistCpOptionSwitch.DIFF, useDiff);
     this.useDiff = useDiff;
     this.fromSnapshot = fromSnapshot;
     this.toSnapshot = toSnapshot;
@@ -314,7 +308,6 @@ public class DistCpOptions {
    * @param skipCRC - boolean switch
    */
   public void setSkipCRC(boolean skipCRC) {
-    validate(DistCpOptionSwitch.SKIP_CRC, skipCRC);
     this.skipCRC = skipCRC;
   }
 
@@ -551,20 +544,15 @@ public class DistCpOptions {
     this.filtersFile = filtersFilename;
   }
 
-  public void validate(DistCpOptionSwitch option, boolean value) {
+  void validate() {
-
+    if (useDiff && deleteMissing) {
-    boolean syncFolder = (option == DistCpOptionSwitch.SYNC_FOLDERS ?
+      // -delete and -diff are mutually exclusive. For backward compatibility,
-        value : this.syncFolder);
+      // we ignore the -delete option here, instead of throwing an
-    boolean overwrite = (option == DistCpOptionSwitch.OVERWRITE ?
+      // IllegalArgumentException. See HDFS-10397 for more discussion.
-        value : this.overwrite);
+      OptionsParser.LOG.warn("-delete and -diff are mutually exclusive. " +
-    boolean deleteMissing = (option == DistCpOptionSwitch.DELETE_MISSING ?
+          "The -delete option will be ignored.");
-        value : this.deleteMissing);
+      setDeleteMissing(false);
-    boolean atomicCommit = (option == DistCpOptionSwitch.ATOMIC_COMMIT ?
+    }
-        value : this.atomicCommit);
-    boolean skipCRC = (option == DistCpOptionSwitch.SKIP_CRC ?
-        value : this.skipCRC);
-    boolean append = (option == DistCpOptionSwitch.APPEND ? value : this.append);
-    boolean useDiff = (option == DistCpOptionSwitch.DIFF ? value : this.useDiff);
 
     if (syncFolder && atomicCommit) {
       throw new IllegalArgumentException("Atomic commit can't be used with " +
@@ -593,7 +581,7 @@ public class DistCpOptions {
       throw new IllegalArgumentException(
           "Append is disallowed when skipping CRC");
     }
-    if ((!syncFolder || deleteMissing) && useDiff) {
+    if (!syncFolder && useDiff) {
       throw new IllegalArgumentException(
           "Diff is valid only with update options");
     }

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

@@ -41,7 +41,7 @@ import com.google.common.base.Preconditions;
  */
 public class OptionsParser {
 
-  private static final Log LOG = LogFactory.getLog(OptionsParser.class);
+  static final Log LOG = LogFactory.getLog(OptionsParser.class);
 
   private static final Options cliOptions = new Options();
 
@@ -88,14 +88,26 @@ public class OptionsParser {
 
     DistCpOptions option = parseSourceAndTargetPaths(command);
 
-    //Process all the other option switches and set options appropriately
+    option.setIgnoreFailures(
-    if (command.hasOption(DistCpOptionSwitch.IGNORE_FAILURES.getSwitch())) {
+        command.hasOption(DistCpOptionSwitch.IGNORE_FAILURES.getSwitch()));
-      option.setIgnoreFailures(true);
-    }
 
-    if (command.hasOption(DistCpOptionSwitch.ATOMIC_COMMIT.getSwitch())) {
+    option.setAtomicCommit(
-      option.setAtomicCommit(true);
+        command.hasOption(DistCpOptionSwitch.ATOMIC_COMMIT.getSwitch()));
-    }
+
+    option.setSyncFolder(
+        command.hasOption(DistCpOptionSwitch.SYNC_FOLDERS.getSwitch()));
+
+    option.setOverwrite(
+        command.hasOption(DistCpOptionSwitch.OVERWRITE.getSwitch()));
+
+    option.setAppend(
+        command.hasOption(DistCpOptionSwitch.APPEND.getSwitch()));
+
+    option.setDeleteMissing(
+        command.hasOption(DistCpOptionSwitch.DELETE_MISSING.getSwitch()));
+
+    option.setSkipCRC(
+        command.hasOption(DistCpOptionSwitch.SKIP_CRC.getSwitch()));
 
     if (command.hasOption(DistCpOptionSwitch.WORK_PATH.getSwitch()) &&
         option.shouldAtomicCommit()) {
@@ -111,25 +123,6 @@ public class OptionsParser {
       option.setLogPath(new Path(getVal(command, DistCpOptionSwitch.LOG_PATH.getSwitch())));
     }
 
-    if (command.hasOption(DistCpOptionSwitch.SYNC_FOLDERS.getSwitch())) {
-      option.setSyncFolder(true);
-    }
-
-    if (command.hasOption(DistCpOptionSwitch.OVERWRITE.getSwitch())) {
-      option.setOverwrite(true);
-    }
-
-    if (command.hasOption(DistCpOptionSwitch.APPEND.getSwitch())) {
-      option.setAppend(true);
-    }
-
-    if (command.hasOption(DistCpOptionSwitch.DELETE_MISSING.getSwitch())) {
-      option.setDeleteMissing(true);
-    }
-
-    if (command.hasOption(DistCpOptionSwitch.SKIP_CRC.getSwitch())) {
-      option.setSkipCRC(true);
-    }
 
     if (command.hasOption(DistCpOptionSwitch.BLOCKING.getSwitch())) {
       option.setBlocking(false);
@@ -164,6 +157,8 @@ public class OptionsParser {
           DistCpOptionSwitch.FILTERS.getSwitch()));
     }
 
+    option.validate();
+
     return option;
   }
 

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

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.tools;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.fail;
 
 import org.junit.Assert;
@@ -691,11 +692,25 @@ public class TestOptionsParser {
     }
 
     try {
-      OptionsParser.parse(new String[] { "-diff", "s1", "s2", "-update", "-delete",
+      options = OptionsParser.parse(new String[] {
+          "-diff", "s1", "s2", "-update", "-delete",
           "hdfs://localhost:9820/source/first",
           "hdfs://localhost:9820/target/" });
-      fail("-diff should fail if -delete option is specified");
+      assertFalse("-delete should be ignored when -diff is specified",
+          options.shouldDeleteMissing());
     } catch (IllegalArgumentException e) {
+      fail("Got unexpected IllegalArgumentException: " + e.getMessage());
+    }
+
+    try {
+      options = OptionsParser.parse(new String[] {
+          "-diff", "s1", "s2", "-delete",
+          "hdfs://localhost:9820/source/first",
+          "hdfs://localhost:9820/target/" });
+      fail("-diff should fail if -update option is not specified");
+    } catch (IllegalArgumentException e) {
+      assertFalse("-delete should be ignored when -diff is specified",
+          options.shouldDeleteMissing());
       GenericTestUtils.assertExceptionContains(
           "Diff is valid only with update options", e);
     }