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

(cherry picked from commit 03788d3015)
(cherry picked from commit dd7f5a3b88)

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

@@ -163,7 +163,6 @@ public class DistCpOptions {
    * @param atomicCommit - boolean switch
    */
   public void setAtomicCommit(boolean atomicCommit) {
-    validate(DistCpOptionSwitch.ATOMIC_COMMIT, atomicCommit);
     this.atomicCommit = atomicCommit;
   }
 
@@ -182,7 +181,6 @@ public class DistCpOptions {
    * @param syncFolder - boolean switch
    */
   public void setSyncFolder(boolean syncFolder) {
-    validate(DistCpOptionSwitch.SYNC_FOLDERS, syncFolder);
     this.syncFolder = syncFolder;
   }
 
@@ -201,7 +199,6 @@ public class DistCpOptions {
    * @param deleteMissing - boolean switch
    */
   public void setDeleteMissing(boolean deleteMissing) {
-    validate(DistCpOptionSwitch.DELETE_MISSING, deleteMissing);
     this.deleteMissing = deleteMissing;
   }
 
@@ -257,7 +254,6 @@ public class DistCpOptions {
    * @param overwrite - boolean switch
    */
   public void setOverwrite(boolean overwrite) {
-    validate(DistCpOptionSwitch.OVERWRITE, overwrite);
     this.overwrite = overwrite;
   }
 
@@ -273,7 +269,6 @@ public class DistCpOptions {
    * update option and CRC is not skipped.
    */
   public void setAppend(boolean append) {
-    validate(DistCpOptionSwitch.APPEND, append);
     this.append = append;
   }
 
@@ -290,7 +285,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;
@@ -317,7 +311,6 @@ public class DistCpOptions {
    * @param skipCRC - boolean switch
    */
   public void setSkipCRC(boolean skipCRC) {
-    validate(DistCpOptionSwitch.SKIP_CRC, skipCRC);
     this.skipCRC = skipCRC;
   }
 
@@ -572,20 +565,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 " +
@@ -614,7 +602,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);
@@ -169,6 +162,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;
@@ -400,7 +401,7 @@ public class TestOptionsParser {
     String val = "DistCpOptions{atomicCommit=false, syncFolder=false, "
         + "deleteMissing=false, ignoreFailures=false, overwrite=false, "
         + "skipCRC=false, blocking=true, numListstatusThreads=0, maxMaps=20, "
-        + "mapBandwidth=100.0, sslConfigurationFile='null', "
+        + "mapBandwidth=100, sslConfigurationFile='null', "
         + "copyStrategy='uniformsize', preserveStatus=[], "
         + "preserveRawXattrs=false, atomicWorkPath=null, logPath=null, "
         + "sourceFileListing=abc, sourcePaths=null, targetPath=xyz, "
@@ -704,11 +705,25 @@ public class TestOptionsParser {
     }
 
     try {
-      OptionsParser.parse(new String[] { "-diff", "s1", "s2", "-update", "-delete",
+      options = OptionsParser.parse(new String[] {
-          "hdfs://localhost:8020/source/first",
+          "-diff", "s1", "s2", "-update", "-delete",
-          "hdfs://localhost:8020/target/" });
+          "hdfs://localhost:9820/source/first",
-      fail("-diff should fail if -delete option is specified");
+          "hdfs://localhost:9820/target/" });
+      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);
     }