Skip to content

[SPARK-24319][SPARK SUBMIT] Fix spark-submit execution where no main class is required. #21450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gaborgsomogyi wants to merge 7 commits into from
Closed

[SPARK-24319][SPARK SUBMIT] Fix spark-submit execution where no main class is required. #21450

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a69850b
[SPARK-24319][SPARK SUBMIT] Fix spark-submit execution where no main …
gaborgsomogyi May 29, 2018
9052dff
No argument run-example test added.
gaborgsomogyi May 30, 2018
12a5145
Fixed noargs error message, merged requires flags
gaborgsomogyi Jun 1, 2018
b03e3de
Move buildCommand part under exception handling
gaborgsomogyi Jun 12, 2018
d5a9ca3
Merge branch 'master' into SPARK-24319
gaborgsomogyi Jun 12, 2018
8a8067e
Minor fixes.
gaborgsomogyi Jun 13, 2018
5a737d7
Indent fix
gaborgsomogyi Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions launcher/src/main/java/org/apache/spark/launcher/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.launcher;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -54,10 +55,12 @@ public static void main(String[] argsArray) throws Exception {
String className = args.remove(0);

boolean printLaunchCommand = !isEmpty(System.getenv("SPARK_PRINT_LAUNCH_COMMAND"));
AbstractCommandBuilder builder;
Map<String, String> env = new HashMap<>();
List<String> cmd;
if (className.equals("org.apache.spark.deploy.SparkSubmit")) {
try {
builder = new SparkSubmitCommandBuilder(args);
AbstractCommandBuilder builder = new SparkSubmitCommandBuilder(args);
cmd = buildCommand(builder, env, printLaunchCommand);
} catch (IllegalArgumentException e) {
printLaunchCommand = false;
System.err.println("Error: " + e.getMessage());
Expand All @@ -76,17 +79,12 @@ public static void main(String[] argsArray) throws Exception {
help.add(parser.className);
}
help.add(parser.USAGE_ERROR);
builder = new SparkSubmitCommandBuilder(help);
AbstractCommandBuilder builder = new SparkSubmitCommandBuilder(help);
cmd = buildCommand(builder, env, printLaunchCommand);
}
} else {
builder = new SparkClassCommandBuilder(className, args);
}

Map<String, String> env = new HashMap<>();
List<String> cmd = builder.buildCommand(env);
if (printLaunchCommand) {
System.err.println("Spark Command: " + join(" ", cmd));
System.err.println("========================================");
AbstractCommandBuilder builder = new SparkClassCommandBuilder(className, args);
cmd = buildCommand(builder, env, printLaunchCommand);
}

if (isWindows()) {
Expand All @@ -101,6 +99,22 @@ public static void main(String[] argsArray) throws Exception {
}
}

/**
* Prepare spark commands with the appropriate command builder.
* If printLaunchCommand is set then the commands will be printed to the stderr.
*/
private static List<String> buildCommand(AbstractCommandBuilder builder,
Copy link
Contributor

@vanzin vanzin Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the Spark style for multi-line parameter lists.

Copy link
Contributor Author

@gaborgsomogyi gaborgsomogyi Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Just for the sake of my understanding what does many mean in the following? All?
For Java code, Apache Spark follows Oracle’s Java code conventions. Many Scala guidelines below also apply to Java.

Copy link
Contributor

@vanzin vanzin Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read that as "unless it cannot apply to Java at all, follow the Scala style".

Map<String, String> env,
boolean printLaunchCommand)
throws IOException, IllegalArgumentException {
List<String> cmd = builder.buildCommand(env);
if (printLaunchCommand) {
System.err.println("Spark Command: " + join(" ", cmd));
System.err.println("========================================");
}
return cmd;
}

/**
* Prepare a command line for execution from a Windows batch script.
*
Expand Down
41 changes: 22 additions & 19 deletions launcher/src/main/java/org/apache/spark/launcher/SparkSubmitCommandBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ class SparkSubmitCommandBuilder extends AbstractCommandBuilder {

final List<String> userArgs;
private final List<String> parsedArgs;
private final boolean requiresAppResource;
// Special command means no appResource and no mainClass required
private final boolean isSpecialCommand;
private final boolean isExample;

/**
Expand All @@ -105,7 +106,7 @@ class SparkSubmitCommandBuilder extends AbstractCommandBuilder {
* spark-submit argument list to be modified after creation.
*/
SparkSubmitCommandBuilder() {
this.requiresAppResource = true;
this.isSpecialCommand = false;
this.isExample = false;
this.parsedArgs = new ArrayList<>();
this.userArgs = new ArrayList<>();
Expand Down Expand Up @@ -138,25 +139,30 @@ class SparkSubmitCommandBuilder extends AbstractCommandBuilder {

case RUN_EXAMPLE:
isExample = true;
appResource = SparkLauncher.NO_RESOURCE;
submitArgs = args.subList(1, args.size());
}

this.isExample = isExample;
OptionParser parser = new OptionParser(true);
parser.parse(submitArgs);
this.requiresAppResource = parser.requiresAppResource;
if (!submitArgs.isEmpty()) {
Copy link
Contributor

@vanzin vanzin Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this check? parser.isSpecialCommand is initialized to false, so seems like it's already handled.

Copy link
Contributor Author

@gaborgsomogyi gaborgsomogyi Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as it leads to the same result.

OptionParser parser = new OptionParser(true);
parser.parse(submitArgs);
this.isSpecialCommand = parser.isSpecialCommand;
} else {
this.isSpecialCommand = false;
}
} else {
this.isExample = isExample;
this.requiresAppResource = false;
this.isSpecialCommand = true;
}
}

@Override
public List<String> buildCommand(Map<String, String> env)
throws IOException, IllegalArgumentException {
if (PYSPARK_SHELL.equals(appResource) && requiresAppResource) {
if (PYSPARK_SHELL.equals(appResource) && !isSpecialCommand) {
return buildPySparkShellCommand(env);
} else if (SPARKR_SHELL.equals(appResource) && requiresAppResource) {
} else if (SPARKR_SHELL.equals(appResource) && !isSpecialCommand) {
return buildSparkRCommand(env);
} else {
return buildSparkSubmitCommand(env);
Expand All @@ -166,18 +172,18 @@ public List<String> buildCommand(Map<String, String> env)
List<String> buildSparkSubmitArgs() {
List<String> args = new ArrayList<>();
OptionParser parser = new OptionParser(false);
final boolean requiresAppResource;
final boolean isSpecialCommand;

// If the user args array is not empty, we need to parse it to detect exactly what
// the user is trying to run, so that checks below are correct.
if (!userArgs.isEmpty()) {
parser.parse(userArgs);
requiresAppResource = parser.requiresAppResource;
isSpecialCommand = parser.isSpecialCommand;
} else {
requiresAppResource = this.requiresAppResource;
isSpecialCommand = this.isSpecialCommand;
}

if (!allowsMixedArguments && requiresAppResource) {
if (!allowsMixedArguments && !isSpecialCommand) {
checkArgument(appResource != null, "Missing application resource.");
}

Expand Down Expand Up @@ -229,7 +235,7 @@ List<String> buildSparkSubmitArgs() {
args.add(join(",", pyFiles));
}

if (isExample) {
if (isExample && !isSpecialCommand) {
checkArgument(mainClass != null, "Missing example class name.");
}

Expand Down Expand Up @@ -421,7 +427,7 @@ private List<String> findExamplesJars() {

private class OptionParser extends SparkSubmitOptionParser {

boolean requiresAppResource = true;
boolean isSpecialCommand = false;
private final boolean errorOnUnknownArgs;

OptionParser(boolean errorOnUnknownArgs) {
Expand Down Expand Up @@ -470,17 +476,14 @@ protected boolean handle(String opt, String value) {
break;
case KILL_SUBMISSION:
case STATUS:
requiresAppResource = false;
isSpecialCommand = true;
parsedArgs.add(opt);
parsedArgs.add(value);
break;
case HELP:
case USAGE_ERROR:
requiresAppResource = false;
parsedArgs.add(opt);
break;
case VERSION:
requiresAppResource = false;
isSpecialCommand = true;
parsedArgs.add(opt);
break;
default:
Expand Down
54 changes: 49 additions & 5 deletions launcher/src/test/java/org/apache/spark/launcher/SparkSubmitCommandBuilderSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.spark.launcher;

import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -27,7 +28,10 @@

import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import static org.junit.Assert.*;

public class SparkSubmitCommandBuilderSuite extends BaseSuite {
Expand Down Expand Up @@ -74,8 +78,11 @@ public void testCliHelpAndNoArg() throws Exception {

@Test
public void testCliKillAndStatus() throws Exception {
testCLIOpts(parser.STATUS);
testCLIOpts(parser.KILL_SUBMISSION);
List<String> params = Arrays.asList("driver-20160531171222-0000");
testCLIOpts(null, parser.STATUS, params);
testCLIOpts(null, parser.KILL_SUBMISSION, params);
testCLIOpts(SparkSubmitCommandBuilder.RUN_EXAMPLE, parser.STATUS, params);
testCLIOpts(SparkSubmitCommandBuilder.RUN_EXAMPLE, parser.KILL_SUBMISSION, params);
}

@Test
Expand Down Expand Up @@ -190,6 +197,36 @@ public void testSparkRShell() throws Exception {
env.get("SPARKR_SUBMIT_ARGS"));
}

@Test(expected = IllegalArgumentException.class)
public void testExamplesRunnerNoArg() throws Exception {
Copy link
Member

@BryanCutler BryanCutler May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When no args are given this is what I get as the output:

$ bin/run-example
Error: Must specify a primary resource (JAR or Python or R file)
Run with --help for usage help or --verbose for debug output

It doesn't really make sense asking for a primary resource when running an example, how about just printing usage? This is what spark-submit does with no args

Copy link
Contributor

@vanzin vanzin May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the output should make sense also.

When the launcher library detects an invalid command it should be running spark-submit --usage-error, which should print usage with the overridden description from the run-example script. That's done in Main.java when it catches an IllegalArgumentException.

Seems that's not exactly what's happening here.

Copy link
Contributor Author

@gaborgsomogyi gaborgsomogyi Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it prints out the usage.

List<String> sparkSubmitArgs = Arrays.asList(SparkSubmitCommandBuilder.RUN_EXAMPLE);
Map<String, String> env = new HashMap<>();
buildCommand(sparkSubmitArgs, env);
}

@Test
public void testExamplesRunnerNoMainClass() throws Exception {
testCLIOpts(SparkSubmitCommandBuilder.RUN_EXAMPLE, parser.HELP, null);
testCLIOpts(SparkSubmitCommandBuilder.RUN_EXAMPLE, parser.USAGE_ERROR, null);
testCLIOpts(SparkSubmitCommandBuilder.RUN_EXAMPLE, parser.VERSION, null);
}

@Rule
public ExpectedException testExamplesRunnerWithMasterNoMainClassEx = ExpectedException.none();

@Test
public void testExamplesRunnerWithMasterNoMainClass() throws Exception {
testExamplesRunnerWithMasterNoMainClassEx.expect(IllegalArgumentException.class);
Copy link
Contributor

@vanzin vanzin Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for a try...catch in this case, which would also be less code, but either is fine. But I'd pick a shorter and more generic name for the rule (since it could be reused in other tests if needed later).

Copy link
Contributor Author

@gaborgsomogyi gaborgsomogyi Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

testExamplesRunnerWithMasterNoMainClassEx.expectMessage("Missing example class name.");

List<String> sparkSubmitArgs = Arrays.asList(
SparkSubmitCommandBuilder.RUN_EXAMPLE,
parser.MASTER + "=foo"
);
Map<String, String> env = new HashMap<>();
buildCommand(sparkSubmitArgs, env);
}

@Test
public void testExamplesRunner() throws Exception {
List<String> sparkSubmitArgs = Arrays.asList(
Expand Down Expand Up @@ -344,10 +381,17 @@ private List<String> buildCommand(List<String> args, Map<String, String> env) th
return newCommandBuilder(args).buildCommand(env);
}

private void testCLIOpts(String opt) throws Exception {
List<String> helpArgs = Arrays.asList(opt, "driver-20160531171222-0000");
private void testCLIOpts(String appResource, String opt, List<String> params) throws Exception {
List<String> args = new ArrayList<>();
if (appResource != null) {
args.add(appResource);
}
args.add(opt);
if (params != null) {
args.addAll(params);
}
Map<String, String> env = new HashMap<>();
List<String> cmd = buildCommand(helpArgs, env);
List<String> cmd = buildCommand(args, env);
assertTrue(opt + " should be contained in the final cmd.",
cmd.contains(opt));
}
Expand Down