Author: drew
Date: Wed Jul 7 23:41:46 2010
New Revision: 961538
URL: http://svn.apache.org/viewvc?rev=961538&view=rev
Log:
MAHOUT-404: AbstractJob fixes from
https://issues.apache.org/jira/secure/attachment/12448373/MAHOUT-404.patch
Added:
mahout/trunk/core/src/test/java/org/apache/mahout/common/AbstractJobTest.java
Modified:
mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java
Modified:
mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java
URL:
http://svn.apache.org/viewvc/mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java?rev=961538&r1=961537&r2=961538&view=diff
==============================================================================
--- mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java
(original)
+++ mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java
Wed Jul 7 23:41:46 2010
@@ -124,7 +124,7 @@ public abstract class AbstractJob extend
* @param description
*/
protected void addFlag(String name, String shortName, String description) {
- options.add(buildOption(name, shortName, description, true, false, null));
+ options.add(buildOption(name, shortName, description, false, false, null));
}
/** Add an option to the the set of options this job will parse when
@@ -280,6 +280,8 @@ public abstract class AbstractJob extend
parser.setGroup(group);
parser.setHelpOption(helpOpt);
cmdLine = parser.parse(args);
+
+
} catch (OptionException e) {
log.error(e.getMessage());
CommandLineUtil.printHelpWithGenericOptions(group);
@@ -291,22 +293,39 @@ public abstract class AbstractJob extend
return null;
}
+ try {
+ parseDirectories(cmdLine);
+ } catch (IllegalArgumentException e) {
+ log.error(e.getMessage());
+ CommandLineUtil.printHelpWithGenericOptions(group);
+ return null;
+ }
+
+
Map<String,String> result = new TreeMap<String,String>();
maybePut(result, cmdLine, this.options.toArray(new
Option[this.options.size()]));
- parseDirectories(cmdLine);
-
log.info("Command line arguments: {}", result);
return result;
}
- /** Extract the values of the <code>inputOption</code> and
<code>outputOption</code>
- * if present, otherwise attempt to retrieve the values of
<code>mapred.input.dir</code>
- * and <code>mapred.output.dir</code>. If none of these are set,
- * {...@link #getInputPath()} and {...@link #getOutputPath()} will return
null.
+ /** Obtain input and output directories from command-line options or hadoop
+ * properties. If <code>addInputOption</code> or
<code>addOutputOption</code>
+ * has been called, this method will throw an <code>OptionException</code>
if
+ * no source (command-line or property) for that value is present.
+ * Otherwise, <code>inputPath</code> or <code>outputPath<code> will be
+ * non-null only if specified as a hadoop property. Command-line options
+ * take precedence over hadoop properties.
+ *
* @param cmdLine
+ * @throws IllegalArgumentException if either inputOption is present,
+ * and neither <code>--input</code> nor <code>-Dmapred.input dir</code>
are
+ * specified or outputOption is present and neither <code>--output</code>
+ * nor <code>-Dmapred.output.dir</code> are specified.
*/
- protected void parseDirectories(CommandLine cmdLine) {
+ protected void parseDirectories(CommandLine cmdLine)
+ throws IllegalArgumentException {
+
Configuration conf = getConf();
if (inputOption != null && cmdLine.hasOption(inputOption)) {
@@ -322,14 +341,32 @@ public abstract class AbstractJob extend
if (outputPath == null && conf.get("mapred.output.dir") != null) {
this.outputPath = new Path(conf.get("mapred.output.dir"));
}
+
+ if (inputOption != null && inputPath == null) {
+ throw new IllegalArgumentException("No input specified: " +
+ inputOption.getPreferredName() + " or -Dmapred.input.dir " +
+ "must be provided to specify input directory");
+ }
+
+ if (outputOption != null && outputPath == null) {
+ throw new IllegalArgumentException("No output specified: " +
+ outputOption.getPreferredName() + " or -Dmapred.output.dir " +
+ "must be provided to specify output directory");
+ }
}
protected static void maybePut(Map<String,String> args, CommandLine cmdLine,
Option... opt) {
for (Option o : opt) {
- // nulls are ok, for cases where options are simple flags.
- Object vo = cmdLine.getValue(o);
- String value = (vo == null) ? null : vo.toString();
- args.put(o.getPreferredName(), value);
+
+ // the option appeared on the command-line, or it has a value
+ // (which is likely a default value).
+ if (cmdLine.hasOption(o) || cmdLine.getValue(o) != null) {
+
+ // nulls are ok, for cases where options are simple flags.
+ Object vo = cmdLine.getValue(o);
+ String value = (vo == null) ? null : vo.toString();
+ args.put(o.getPreferredName(), value);
+ }
}
}
Added:
mahout/trunk/core/src/test/java/org/apache/mahout/common/AbstractJobTest.java
URL:
http://svn.apache.org/viewvc/mahout/trunk/core/src/test/java/org/apache/mahout/common/AbstractJobTest.java?rev=961538&view=auto
==============================================================================
---
mahout/trunk/core/src/test/java/org/apache/mahout/common/AbstractJobTest.java
(added)
+++
mahout/trunk/core/src/test/java/org/apache/mahout/common/AbstractJobTest.java
Wed Jul 7 23:41:46 2010
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.mahout.common;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.util.ToolRunner;
+import org.apache.mahout.common.commandline.DefaultOptionCreator;
+import org.junit.Test;
+
+import junit.framework.TestCase;
+
+/**
+ *
+ */
+public class AbstractJobTest {
+
+ interface AbstractJobFactory {
+ public AbstractJob getJob();
+ }
+
+ @Test
+ public void testFlag() throws Exception {
+ final Map<String,String> testMap = new HashMap<String,String>();
+
+ final AbstractJobFactory fact = new AbstractJobFactory() {
+ public AbstractJob getJob() {
+ return new AbstractJob() {
+ @Override
+ public int run(String[] args) throws Exception {
+ addFlag("testFlag", "t", "a simple test flag");
+
+ Map<String,String> argMap = parseArguments(args);
+ testMap.clear();
+ testMap.putAll(argMap);
+ return 1;
+ }
+ };
+ }
+ };
+
+ // testFlag will only be present if speciied on the command-line
+
+ String[] noFlag = new String[0];
+ ToolRunner.run(fact.getJob(), noFlag);
+ TestCase.assertFalse("test map for absent flag",
+ testMap.containsKey("--testFlag"));
+
+ String[] withFlag = { "--testFlag" };
+ ToolRunner.run(fact.getJob(), withFlag);
+ TestCase.assertTrue("test map for present flag",
+ testMap.containsKey("--testFlag"));
+ }
+
+ @Test
+ public void testOptions() throws Exception {
+ final Map<String,String> testMap = new HashMap<String,String>();
+
+ final AbstractJobFactory fact = new AbstractJobFactory() {
+ public AbstractJob getJob() {
+ return new AbstractJob() {
+ @Override
+ public int run(String[] args) throws Exception {
+ this.addOption(DefaultOptionCreator.overwriteOption().create());
+ this.addOption("option", "o", "option");
+ this.addOption("required", "r", "required", true /* required */);
+ this.addOption("notRequired", "nr", "not required", false /* not
required */);
+ this.addOption("hasDefault", "hd", "option w/ default",
"defaultValue");
+
+
+ Map<String,String> argMap = parseArguments(args);
+ if (argMap == null) {
+ return -1;
+ }
+
+ testMap.clear();
+ testMap.putAll(argMap);
+
+ return 0;
+ }
+ };
+ }
+ };
+
+ int ret;
+
+ ret = ToolRunner.run(fact.getJob(), new String[0]);
+ TestCase.assertEquals("-1 for missing required options", -1, ret);
+
+ ret = ToolRunner.run(fact.getJob(), new String[]{
+ "--required", "requiredArg"
+ });
+ TestCase.assertEquals("0 for no missing required options", 0, ret);
+ TestCase.assertEquals("requiredArg", testMap.get("--required"));
+ TestCase.assertEquals("defaultValue", testMap.get("--hasDefault"));
+ TestCase.assertNull(testMap.get("--option"));
+ TestCase.assertNull(testMap.get("--notRequired"));
+ TestCase.assertFalse(testMap.containsKey("--overwrite"));
+
+ ret = ToolRunner.run(fact.getJob(), new String[]{
+ "--required", "requiredArg",
+ "--unknownArg"
+ });
+ TestCase.assertEquals("-1 for including unknown options", -1, ret);
+
+ ret = ToolRunner.run(fact.getJob(), new String[]{
+ "--required", "requiredArg",
+ "--required", "requiredArg2",
+ });
+ TestCase.assertEquals("-1 for including duplicate options", -1, ret);
+
+ ret = ToolRunner.run(fact.getJob(), new String[]{
+ "--required", "requiredArg",
+ "--overwrite",
+ "--hasDefault", "nonDefault",
+ "--option", "optionValue",
+ "--notRequired", "notRequired"
+ });
+ TestCase.assertEquals("0 for no missing required options", 0, ret);
+ TestCase.assertEquals("requiredArg", testMap.get("--required"));
+ TestCase.assertEquals("nonDefault", testMap.get("--hasDefault"));
+ TestCase.assertEquals("optionValue", testMap.get("--option"));
+ TestCase.assertEquals("notRequired", testMap.get("--notRequired"));
+ TestCase.assertTrue(testMap.containsKey("--overwrite"));
+
+ ret = ToolRunner.run(fact.getJob(), new String[]{
+ "-r", "requiredArg",
+ "-ow",
+ "-hd", "nonDefault",
+ "-o", "optionValue",
+ "-nr", "notRequired"
+ });
+ TestCase.assertEquals("0 for no missing required options", 0, ret);
+ TestCase.assertEquals("requiredArg", testMap.get("--required"));
+ TestCase.assertEquals("nonDefault", testMap.get("--hasDefault"));
+ TestCase.assertEquals("optionValue", testMap.get("--option"));
+ TestCase.assertEquals("notRequired", testMap.get("--notRequired"));
+ TestCase.assertTrue(testMap.containsKey("--overwrite"));
+
+ }
+
+ @Test
+ public void testInputOutputPaths() throws Exception {
+
+ final AbstractJobFactory fact = new AbstractJobFactory() {
+ public AbstractJob getJob() {
+ return new AbstractJob() {
+ @Override
+ public int run(String[] args) throws Exception {
+ addInputOption();
+ addOutputOption();
+
+ // arg map should be null if a required option is missing.
+ Map<String, String> argMap = parseArguments(args);
+
+ if (argMap == null) {
+ return -1;
+ }
+
+ Path inputPath = getInputPath();
+ TestCase.assertNotNull("getInputPath() returns non-null",
inputPath);
+
+ Path outputPath = getInputPath();
+ TestCase.assertNotNull("getOutputPath() returns non-null",
outputPath);
+ return 0;
+ }
+ };
+ }
+ };
+
+ AbstractJob job;
+ int ret;
+
+ ret = ToolRunner.run(fact.getJob(), new String[0]);
+ TestCase.assertEquals("-1 for missing input option", -1, ret);
+
+ final String testInputPath = "testInputPath";
+ final String testOutputPath = "testOutputPath";
+ final String testInputPropertyPath = "testInputPropertyPath";
+ final String testOutputPropertyPath = "testOutputPropertyPath";
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "--input", testInputPath });
+ TestCase.assertEquals("-1 for missing output option", -1, ret);
+ TestCase.assertEquals("input path is correct", testInputPath,
+ job.getInputPath().toString());
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "--output", testOutputPath });
+ TestCase.assertEquals("-1 for missing input option", -1, ret);
+ TestCase.assertEquals("output path is correct", testOutputPath,
+ job.getOutputPath().toString());
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "--input", testInputPath, "--output", testOutputPath });
+ TestCase.assertEquals("0 for complete options", 0, ret);
+ TestCase.assertEquals("input path is correct", testInputPath,
+ job.getInputPath().toString());
+ TestCase.assertEquals("output path is correct", testOutputPath,
+ job.getOutputPath().toString());
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "--input", testInputPath, "--output", testOutputPath });
+ TestCase.assertEquals("0 for complete options", 0, ret);
+ TestCase.assertEquals("input path is correct", testInputPath,
+ job.getInputPath().toString());
+ TestCase.assertEquals("output path is correct", testOutputPath,
+ job.getOutputPath().toString());
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "-Dmapred.input.dir=" + testInputPropertyPath,
+ "-Dmapred.output.dir=" + testOutputPropertyPath });
+ TestCase.assertEquals("0 for complete options", 0, ret);
+ TestCase.assertEquals("input path from property is correct",
+ testInputPropertyPath, job.getInputPath().toString());
+ TestCase.assertEquals("output path from property is correct",
+ testOutputPropertyPath, job.getOutputPath().toString());
+
+ job = fact.getJob();
+ ret = ToolRunner.run(job, new String[]{
+ "-Dmapred.input.dir=" + testInputPropertyPath,
+ "-Dmapred.output.dir=" + testOutputPropertyPath,
+ "--input", testInputPath,
+ "--output", testOutputPath });
+ TestCase.assertEquals("input command-line option precedes property",
+ testInputPath, job.getInputPath().toString());
+ TestCase.assertEquals("output command-line option precedes property",
+ testOutputPath, job.getOutputPath().toString());
+ }
+}
