Fix propagation of properties file arg.

Also add a test. And remove some hardcoded strings, using the constants
defined in SparkSubmitOptionParser.

Author: Marcelo Vanzin

launcher/src/main/java/org/apache/spark/launcher/SparkClassCommandBuilder.java

@@ -112,10 +112,8 @@ private List<String> createSparkSubmitCommand(Map<String, String> env) throws IO
     final List<String> sparkSubmitArgs = new ArrayList<String>();
     final List<String> appArgs = new ArrayList<String>();
 
-    // This parser exists for two reasons:
-    // - to expose the command line args constants, since they're not static
-    // - to special-case the HELP command line argument, and allow it to be propagated to
-    //   the app being launched.
+    // Parse the command line and special-case the HELP command line argument, allowing it to be
+    // propagated to the app being launched.
     SparkSubmitOptionParser parser = new SparkSubmitOptionParser() {
 
       @Override

launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java

@@ -135,6 +135,7 @@ public SparkLauncher setSparkHome(String sparkHome) {
    */
   public SparkLauncher setPropertiesFile(String path) {
     checkNotNull(path, "path");
+    this.propertiesFile = path;
     return this;
   }
 
@@ -504,53 +505,54 @@ SparkLauncher addSparkArgs(String... args) {
   // Visible for testing.
   List<String> buildSparkSubmitArgs() {
     List<String> args = new ArrayList<String>();
+    SparkSubmitOptionParser parser = new SparkSubmitOptionParser();
 
     if (verbose) {
-      args.add("--verbose");
+      args.add(parser.VERBOSE);
     }
 
     if (master != null) {
-      args.add("--master");
+      args.add(parser.MASTER);
       args.add(master);
     }
 
     if (deployMode != null) {
-      args.add("--deploy-mode");
+      args.add(parser.DEPLOY_MODE);
       args.add(deployMode);
     }
 
     if (appName != null) {
-      args.add("--name");
+      args.add(parser.NAME);
       args.add(appName);
     }
 
     for (Map.Entry<String, String> e : conf.entrySet()) {
-      args.add("--conf");
+      args.add(parser.CONF);
       args.add(String.format("%s=%s", e.getKey(), e.getValue()));
     }
 
     if (propertiesFile != null) {
-      args.add("--properties-file");
+      args.add(parser.PROPERTIES_FILE);
       args.add(propertiesFile);
     }
 
     if (!jars.isEmpty()) {
-      args.add("--jars");
+      args.add(parser.JARS);
       args.add(join(",", jars));
     }
 
     if (!files.isEmpty()) {
-      args.add("--files");
+      args.add(parser.FILES);
       args.add(join(",", files));
     }
 
     if (!pyFiles.isEmpty()) {
-      args.add("--py-files");
+      args.add(parser.PY_FILES);
       args.add(join(",", pyFiles));
     }
 
     if (mainClass != null) {
-      args.add("--class");
+      args.add(parser.CLASS);
       args.add(mainClass);
     }
 

launcher/src/main/java/org/apache/spark/launcher/SparkSubmitOptionParser.java

@@ -30,13 +30,14 @@
  * is a single list of options that needs to be maintained (well, sort of, but it makes it harder
  * to break things).
  */
-abstract class SparkSubmitOptionParser {
+class SparkSubmitOptionParser {
 
   // The following constants define the "main" name for the available options. They're defined
   // to avoid copy & paste of the raw strings where they're needed.
   //
   // The fields are not static so that they're exposed to Scala code that uses this class. See
-  // SparkSubmitArguments.scala.
+  // SparkSubmitArguments.scala. That is also why this class is not abstract - to allow code to
+  // easily use these constants without having to create dummy implementations of this class.
   protected final String CLASS = "--class";
   protected final String CONF = "--conf";
   protected final String DEPLOY_MODE = "--deploy-mode";
@@ -186,15 +187,19 @@ protected final void parse(List<String> args) {
    * @param value The value. This will be <i>null</i> if the option does not take a value.
    * @return Whether to continue parsing the argument list.
    */
-  protected abstract boolean handle(String opt, String value);
+  protected boolean handle(String opt, String value) {
+    throw new UnsupportedOperationException();
+  }
 
   /**
    * Callback for when an unrecognized option is parsed.
    *
    * @param opt Unrecognized option from the command line.
    * @return Whether to continue parsing the argument list.
    */
-  protected abstract boolean handleUnknown(String opt);
+  protected boolean handleUnknown(String opt) {
+    throw new UnsupportedOperationException();
+  }
 
   /**
    * Callback for remaining command line arguments after either {@link #handle(String, String)} or
@@ -203,7 +208,9 @@ protected final void parse(List<String> args) {
    *
    * @param extra List of remaining arguments.
    */
-  protected abstract void handleExtraArgs(List<String> extra);
+  protected void handleExtraArgs(List<String> extra) {
+    throw new UnsupportedOperationException();
+  }
 
   private String findCliOption(String name, String[][] available) {
     for (String[] candidates : available) {

launcher/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java

@@ -26,6 +26,8 @@
 import java.util.Map;
 import java.util.regex.Pattern;
 
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -38,6 +40,18 @@ public class SparkLauncherSuite {
 
   private static final Logger LOG = LoggerFactory.getLogger(SparkLauncherSuite.class);
 
+  private static File dummyPropsFile;
+
+  @BeforeClass
+  public static void setUp() throws Exception {
+    dummyPropsFile = File.createTempFile("spark", "properties");
+  }
+
+  @AfterClass
+  public static void cleanUp() throws Exception {
+    dummyPropsFile.delete();
+  }
+
   @Test
   public void testDriverCmdBuilder() throws Exception {
     testCmdBuilder(true);
@@ -75,6 +89,7 @@ private void testCmdBuilder(boolean isDriver) throws Exception {
       .setAppResource("/foo")
       .setAppName("MyApp")
       .setMainClass("my.Class")
+      .setPropertiesFile(dummyPropsFile.getAbsolutePath())
       .addAppArgs("foo", "bar")
       .setConf(SparkLauncher.DRIVER_MEMORY, "1g")
       .setConf(SparkLauncher.DRIVER_EXTRA_CLASSPATH, "/driver")
@@ -128,11 +143,13 @@ private void testCmdBuilder(boolean isDriver) throws Exception {
     }
 
     // Checks below are the same for both driver and non-driver mode.
+    SparkSubmitOptionParser parser = new SparkSubmitOptionParser();
 
-    assertEquals("yarn", findArgValue(cmd, "--master"));
-    assertEquals(deployMode, findArgValue(cmd, "--deploy-mode"));
-    assertEquals("my.Class", findArgValue(cmd, "--class"));
-    assertEquals("MyApp", findArgValue(cmd, "--name"));
+    assertEquals(dummyPropsFile.getAbsolutePath(), findArgValue(cmd, parser.PROPERTIES_FILE));
+    assertEquals("yarn", findArgValue(cmd, parser.MASTER));
+    assertEquals(deployMode, findArgValue(cmd, parser.DEPLOY_MODE));
+    assertEquals("my.Class", findArgValue(cmd, parser.CLASS));
+    assertEquals("MyApp", findArgValue(cmd, parser.NAME));
 
     boolean appArgsOk = false;
     for (int i = 0; i < cmd.size(); i++) {
@@ -146,7 +163,7 @@ private void testCmdBuilder(boolean isDriver) throws Exception {
     }
     assertTrue("App resource and args should be added to command.", appArgsOk);
 
-    Map<String, String> conf = parseConf(cmd);
+    Map<String, String> conf = parseConf(cmd, parser);
     assertEquals("foo", conf.get("spark.foo"));
   }
 
@@ -178,10 +195,10 @@ private boolean contains(String needle, Iterable<String> haystack) {
     return false;
   }
 
-  private Map<String, String> parseConf(List<String> cmd) {
+  private Map<String, String> parseConf(List<String> cmd, SparkSubmitOptionParser parser) {
     Map<String, String> conf = new HashMap<String, String>();
     for (int i = 0; i < cmd.size(); i++) {
-      if (cmd.get(i).equals("--conf")) {
+      if (cmd.get(i).equals(parser.CONF)) {
         String[] val = cmd.get(i + 1).split("=", 2);
         conf.put(val[0], val[1]);
         i += 1;