Skip to content

Conversation

@taoli91
Copy link
Contributor

@taoli91 taoli91 commented Apr 26, 2016

What changes were proposed in this pull request?

Windows has a limitation of 8192 characters on command line. This limitation will fail the test cases related to command builder in some test cases. This pull request adopt the method in Hadoop, which create a Jar that ref all the dependencies in the manifest to shorten the class path passing to the command line.

How was this patch tested?

Unit tests on windows and Linux. Note that this commit can't fix all the unit test errors on windows.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

* Create a jar file at the given path, containing a manifest with a classpath
* that references all specified entries.
*/
def createShortClassPath(tempDir: File, classPath: String) : String = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, def createShortClassPath(tempDir: File, classPath: String): String = {

@vanzin
Copy link
Contributor

vanzin commented Aug 5, 2016

I'm not a fan of this approach. This means that every invocation of the launcher code will create temp files and have to deal with cleaning it up; except the launcher cannot do it, because the jvm running SparkSubmit needs it. And now SparkSubmit needs to know to clean it up. That's adding a lot of complexity.

Setting the driver / executor classpaths in tests should be mostly unnecessary at this point, since the root pom file sets SPARK_DIST_CLASSPATH and that should be sufficient to propagate the needed jars to all child processes.

For the generic use case it is possible for someone could create a command line that exceeds the limit; but I think a different approach would be better. Maybe generating a temp config file, with the contents of both the user-provided config and the defaults from Spark's conf dir, but that has the same problem with "who deletes the temp conf file".

On a side note, there's a ton of style issues with your patch, please try to follow the style of the code you're modifying.

@vanzin
Copy link
Contributor

vanzin commented Aug 5, 2016

Update: I thought about using argument files, but they're only supported by javac (javac @argfile) and not by java. So if fixing anything, I'm in favor of:

  • removing the use of "driverJavaOptions" from unit tests since it shouldn't be needed - SPARK_DIST_CLASSPATH already handles it
  • live with the fact that SparkLauncher's generated command line is limited to 8k characters on Windows

@srowen
Copy link
Member

srowen commented Aug 15, 2016

Let's close this PR

srowen added a commit to srowen/spark that referenced this pull request Aug 27, 2016
@asfgit asfgit closed this in 1a48c00 Aug 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants