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

gaborgsomogyi · 2018-05-29T15:58:07Z

What changes were proposed in this pull request?

With PR 20925 now it's not possible to execute the following commands:

run-example
run-example --help
run-example --version
run-example --usage-error
run-example --status ...
run-example --kill ...

In this PR the execution will be allowed for the mentioned commands.

How was this patch tested?

Existing unit tests extended + additional written.

…class is required.

gaborgsomogyi · 2018-05-29T17:30:26Z

test this please

SparkQA · 2018-05-29T20:19:35Z

Test build #91262 has finished for PR 21450 at commit a69850b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2018-05-29T20:35:14Z

cc @vanzin

SparkQA · 2018-05-29T22:07:30Z

Test build #91267 has finished for PR 21450 at commit a69850b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-05-29T22:36:07Z

This doesn't seem to be addressing the issue reported in the bug. The exact same error happens with your patch:

$ ./bin/run-example 
Exception in thread "main" java.lang.IllegalArgumentException: Missing application resource.
        at org.apache.spark.launcher.CommandBuilderUtils.checkArgument(CommandBuilderUtils.java:241)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildSparkSubmitArgs(SparkSubmitCommandBuilder.java:185)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildSparkSubmitCommand(SparkSubmitCommandBuilder.java:300)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildCommand(SparkSubmitCommandBuilder.java:166)
        at org.apache.spark.launcher.Main.main(Main.java:86)

gaborgsomogyi · 2018-05-30T10:04:18Z

Updated the description to reflect the no arg case as well.

SparkQA · 2018-05-30T14:20:03Z

Test build #91299 has finished for PR 21450 at commit 9052dff.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-05-30T17:19:12Z

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

@@ -229,7 +238,7 @@
      args.add(join(",", pyFiles));
    }

-    if (isExample) {
+    if (isExample && requiresMainClass) {


In what situation will requiresAppResource be different from requiresMainClass at this point?

It seems to me they're basically the same. The only point in your code where they differ is in the no-arg constructor, which is not used by the run-example script.

Seems to me like you could rename the existing variable to isSpecialCommand or something and use it instead for both cases.

Originally I was hesitating to merge them and loose the 2 different meaning but it's better. Added a small comment.

vanzin · 2018-05-30T17:19:58Z

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

-  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> helpArgs = new ArrayList<>();


Since you're changing this, probably better to rename the variable since these are not help arguments.

BryanCutler · 2018-05-30T21:26:24Z

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

@@ -190,6 +194,23 @@ public void testSparkRShell() throws Exception {
      env.get("SPARKR_SUBMIT_ARGS"));
  }

+  @Test
+  public void testExamplesRunnerNoArg() throws Exception {


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

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.

Now it prints out the usage.

SparkQA · 2018-06-01T17:27:24Z

Test build #91390 has finished for PR 21450 at commit 12a5145.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-06-04T17:31:30Z

This is still missing something.

$ ./bin/run-example --master local
Exception in thread "main" java.lang.IllegalArgumentException: Missing application resource.
        at org.apache.spark.launcher.CommandBuilderUtils.checkArgument(CommandBuilderUtils.java:241)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildSparkSubmitArgs(SparkSubmitCommandBuilder.java:189)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildSparkSubmitCommand(SparkSubmitCommandBuilder.java:304)
        at org.apache.spark.launcher.SparkSubmitCommandBuilder.buildCommand(SparkSubmitCommandBuilder.java:170)
        at org.apache.spark.launcher.Main.main(Main.java:86)

HyukjinKwon · 2018-06-09T08:52:20Z

ok to test

SparkQA · 2018-06-09T08:58:12Z

Test build #91598 has finished for PR 21450 at commit 12a5145.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-12T09:13:50Z

Test build #91698 has finished for PR 21450 at commit b03e3de.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi · 2018-06-12T09:21:40Z

The issue seems unrelated.

[error] (kubernetes-integration-tests/*:checkstyle) java.io.FileNotFoundException: checkstyle-config.xml (No such file or directory)

Trying to rebase the changes to the latest master.

SparkQA · 2018-06-12T09:33:50Z

Test build #91700 has finished for PR 21450 at commit d5a9ca3.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
logInfo(s\"Using output committer class $
public class JavaPowerIterationClusteringExample
class GBTClassificationModel(TreeEnsembleModel, JavaClassificationModel, JavaMLWritable,
class PowerIterationClustering(HasMaxIter, HasWeightCol, JavaParams, JavaMLReadable,
class PrefixSpan(JavaParams):
public class MaskExpressionsUtils
case class ArrayRemove(left: Expression, right: Expression)
trait MaskLike
trait MaskLikeWithN extends MaskLike
case class Mask(child: Expression, upper: String, lower: String, digit: String)
case class MaskFirstN(
case class MaskLastN(
case class MaskShowFirstN(
case class MaskShowLastN(
case class MaskHash(child: Expression)
abstract class FileFormatDataWriter(
class EmptyDirectoryDataWriter(
class SingleDirectoryDataWriter(
class DynamicPartitionDataWriter(
class WriteJobDescription(
case class WriteTaskResult(commitMsg: TaskCommitMessage, summary: ExecutedWriteSummary)
case class ExecutedWriteSummary(

vanzin · 2018-06-12T23:09:26Z

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

-      OptionParser parser = new OptionParser(true);
-      parser.parse(submitArgs);
-      this.requiresAppResource = parser.requiresAppResource;
+      if (!submitArgs.isEmpty()) {


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

Removed as it leads to the same result.

vanzin · 2018-06-12T23:10:02Z

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

+   * 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,


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

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.

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

vanzin · 2018-06-12T23:16:26Z

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

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


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).

SparkQA · 2018-06-13T13:54:02Z

Test build #91763 has finished for PR 21450 at commit 8a8067e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-06-13T17:10:58Z

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

+   * If printLaunchCommand is set then the commands will be printed to the stderr.
+   */
+  private static List<String> buildCommand(
+    AbstractCommandBuilder builder,


The style is to double indent parameter lists... when in doubt, look at the rest of the code, not the style guide.

SparkQA · 2018-06-14T19:35:26Z

Test build #91853 has finished for PR 21450 at commit 5a737d7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-06-14T21:54:18Z

Looks good. "run-example --kill blah" is kinda weird, but not worth it to handle that case.

Merging to master.

[SPARK-24319][SPARK SUBMIT] Fix spark-submit execution where no main …

a69850b

…class is required.

No argument run-example test added.

9052dff

vanzin reviewed May 30, 2018

View reviewed changes

BryanCutler reviewed May 30, 2018

View reviewed changes

Fixed noargs error message, merged requires flags

12a5145

Move buildCommand part under exception handling

b03e3de

Merge branch 'master' into SPARK-24319

d5a9ca3

vanzin reviewed Jun 12, 2018

View reviewed changes

Minor fixes.

8a8067e

vanzin reviewed Jun 13, 2018

View reviewed changes

Indent fix

5a737d7

asfgit closed this in 18cb0c0 Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gaborgsomogyi commented May 29, 2018 •

edited

Loading

gaborgsomogyi commented May 29, 2018

SparkQA commented May 29, 2018

gaborgsomogyi commented May 29, 2018

SparkQA commented May 29, 2018

vanzin commented May 29, 2018

gaborgsomogyi commented May 30, 2018

SparkQA commented May 30, 2018

vanzin May 30, 2018

gaborgsomogyi Jun 1, 2018

vanzin May 30, 2018

gaborgsomogyi Jun 1, 2018

BryanCutler May 30, 2018 •

edited

Loading

vanzin May 30, 2018

gaborgsomogyi Jun 1, 2018

SparkQA commented Jun 1, 2018

vanzin commented Jun 4, 2018

HyukjinKwon commented Jun 9, 2018

SparkQA commented Jun 9, 2018

SparkQA commented Jun 12, 2018

gaborgsomogyi commented Jun 12, 2018

SparkQA commented Jun 12, 2018

vanzin Jun 12, 2018

gaborgsomogyi Jun 13, 2018

vanzin Jun 12, 2018

gaborgsomogyi Jun 13, 2018

vanzin Jun 13, 2018

vanzin Jun 12, 2018

gaborgsomogyi Jun 13, 2018

SparkQA commented Jun 13, 2018

vanzin Jun 13, 2018

SparkQA commented Jun 14, 2018

vanzin commented Jun 14, 2018

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

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

Conversation

gaborgsomogyi commented May 29, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

gaborgsomogyi commented May 29, 2018

SparkQA commented May 29, 2018

gaborgsomogyi commented May 29, 2018

SparkQA commented May 29, 2018

vanzin commented May 29, 2018

gaborgsomogyi commented May 30, 2018

SparkQA commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 1, 2018

vanzin commented Jun 4, 2018

HyukjinKwon commented Jun 9, 2018

SparkQA commented Jun 9, 2018

SparkQA commented Jun 12, 2018

gaborgsomogyi commented Jun 12, 2018

SparkQA commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 13, 2018

Choose a reason for hiding this comment

SparkQA commented Jun 14, 2018

vanzin commented Jun 14, 2018

gaborgsomogyi commented May 29, 2018 •

edited

Loading

BryanCutler May 30, 2018 •

edited

Loading