diff --git a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpOptions.java b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpOptions.java index 1e8a20cc532..2a962a61442 100644 --- a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpOptions.java +++ b/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) { - - boolean syncFolder = (option == DistCpOptionSwitch.SYNC_FOLDERS ? - value : this.syncFolder); - boolean overwrite = (option == DistCpOptionSwitch.OVERWRITE ? - value : this.overwrite); - boolean deleteMissing = (option == DistCpOptionSwitch.DELETE_MISSING ? - value : this.deleteMissing); - 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); + void validate() { + if (useDiff && deleteMissing) { + // -delete and -diff are mutually exclusive. For backward compatibility, + // we ignore the -delete option here, instead of throwing an + // IllegalArgumentException. See HDFS-10397 for more discussion. + OptionsParser.LOG.warn("-delete and -diff are mutually exclusive. " + + "The -delete option will be ignored."); + setDeleteMissing(false); + } 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"); } diff --git a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/OptionsParser.java b/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/OptionsParser.java index 37add1edac4..414068c4d5a 100644 --- a/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/OptionsParser.java +++ b/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 - if (command.hasOption(DistCpOptionSwitch.IGNORE_FAILURES.getSwitch())) { - option.setIgnoreFailures(true); - } + option.setIgnoreFailures( + command.hasOption(DistCpOptionSwitch.IGNORE_FAILURES.getSwitch())); - if (command.hasOption(DistCpOptionSwitch.ATOMIC_COMMIT.getSwitch())) { - option.setAtomicCommit(true); - } + option.setAtomicCommit( + 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; } diff --git a/hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestOptionsParser.java b/hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestOptionsParser.java index 389a8ea6e8d..946722176cd 100644 --- a/hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestOptionsParser.java +++ b/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", - "hdfs://localhost:8020/source/first", - "hdfs://localhost:8020/target/" }); - fail("-diff should fail if -delete option is specified"); + options = OptionsParser.parse(new String[] { + "-diff", "s1", "s2", "-update", "-delete", + "hdfs://localhost:9820/source/first", + "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); }