[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

tsudukim · 2015-03-27T09:49:25Z

Modified to accept double-quotated args properly in spark-shell.cmd.

…path Modified to accept multiple jars in spark-shell.cmd.

SparkQA · 2015-03-27T11:19:02Z

Test build #29297 has finished for PR 5227 at commit 04f4291.

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

srowen · 2015-03-27T12:03:10Z

I tend to trust your judgment on modifying these Windows scripts, especially if you've verified that this works where it did not before. CC @nishkamravi2 and @vanzin since this will touch the scripts also being modified in #5085

vanzin · 2015-03-27T17:03:18Z

I think the real problem here is that , is a delimiter in Windows batch scripts and thus needs to be escaped:
http://ss64.com/nt/syntax-esc.html

So I think the correct fix would be to change CommandBuilderUtils.quoteForBatchScript to escape commas also. That would also avoid the temp file... which can cause problems if two jobs are being launched at the same time.

vanzin · 2015-03-27T18:59:19Z

Ah, and even though jenkins doesn't run unit tests on Windows, it would be nice to update SparkLauncherSuite to add a unit test for this, since that will exercise this code path (regardless of which fix).

srowen · 2015-03-28T07:01:44Z

OK @vanzin sounds like you recommend we not make this change. @tsudukim not sure if you feel comfortable trying to make the change he suggests?

vanzin · 2015-03-30T22:26:25Z

Hi @tsudukim , are you planning on trying my suggestion? Otherwise I can make the change and test it.

tsudukim · 2015-03-31T02:13:36Z

@vanzin I'm not sure I have got your suggestion right, but as I wrote in JIRA, I think this is not the Java side problem.
https://issues.apache.org/jira/browse/SPARK-6435
Exactly the , is the problem. When Java application receives the arguments, they are already splitted by comma. If we want to escape comma, we should do it in batch side.

tsudukim · 2015-03-31T02:24:08Z

Java application means the launcher launcher\src\main\java\org\apache\spark\launcher\Main.java.

Conflicts: bin/spark-class2.cmd

- added random string to the temporary filename - double-quotation followed by `cmd /c` did not worked properly - no need to escape `=` by `^` - if double-quoted string ended with `\` like classpath, the last `\` is parsed as the escape charactor and the closing `"` didn't work properly

tsudukim · 2015-04-02T06:18:41Z

Ah @vanzin, I didn't understand your suggestion. CommandBuilderUtils needs modified to escape comma.
But I think we still need to modify spark-class2.cmd as well.

When we execute bin\spark-shell.cmd --jars "C:\Path to\jar1.jar,C:\Path to\jar2.jar",
we get following with original spark-class2.cmd even if CommandBuilderUtils is fixed to escape comma.
... --jars "C:\Path to\jar1.jar C:\Path to\jar2.jar" ...
but we get with my PR spark-class2.cmd
... --jars "C:\Path to\jar1.jar,C:\Path to\jar2.jar" ...

Two job at the same time is surely a problem, so added random string to the filename to reduce the probability this problem occurs.

I know it's better to solve this problem without using temporary file if possible, but I have no idea now. If you've got any suggestion, please let me know.
I'd really like to replace all *.cmd to powershell rather than write ugly codes like this....

SparkQA · 2015-04-02T07:46:51Z

Test build #29590 has finished for PR 5227 at commit 0d4dc41.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

tsudukim · 2015-04-02T08:19:33Z

oops, forgot to include fixed test code.

srowen · 2015-04-02T08:44:40Z

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

      default:
        break;
      }
      quoted.appendCodePoint(cp);
    }
+    if (arg.codePointAt(arg.length() - 1) == '\\') {
+        quoted.append("\\");


This is indented too far. Why would a backslash need escaping only in the final position?

In batch, backslash \ followed by double-quotation " is parsed as a escape charactor.
For example, \" means " itself.
We face a problem in this case:
java.exe -cp "C:\path\to\directory\" ...
The closing " doesn't work properly, so we should escape only the last \.

Oh, I assumed you would escape all backslashes if they are the escape character. Is that not necessary?

Backslash is escape character only when followed by ". I assumed it's the only case when the string ends with backslash.

OK, so it's not a general escape character, got it.

SparkQA · 2015-04-02T09:49:27Z

Test build #29599 has finished for PR 5227 at commit 1fee420.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

vanzin · 2015-04-03T18:42:31Z

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

@@ -260,15 +260,14 @@ static String quoteForBatchScript(String arg) {
        quoted.append('"');
        break;

-      case '=':


Did you run SparkLauncherSuite on Windows? I added this specifically to fix an issue with that path (see 92a9cfb).

I've run SparkLauncherSuite on Windows and it's OK.
If double-quotation is parsed properly, = in double-quotation is not need to be escaped.

vanzin · 2015-04-03T18:43:52Z

I tried to figure out a solution that didn't involve a temp file, but failed. Guess I'll just delegate - change looks fine except for the one comment I left about testing.

srowen · 2015-04-06T10:27:46Z

@tsudukim back to you regarding the test case. I'd like to get this in one way or the other since it's an important fix for Windows.

srowen · 2015-04-12T12:32:58Z

@tsudukim Ping? I think this still has an outstanding question from Marcelo above that should be addressed.

tsudukim · 2015-04-13T07:35:56Z

Ah, sorry to be late... I didn't have time to struggle with it. I'll do it this week.

srowen · 2015-04-22T10:58:41Z

Is this one still active? I'd like to figure out whether this is stalled or might proceed.

…/SPARK-6435-2 Conflicts: bin/spark-class2.cmd

tsudukim · 2015-04-24T04:28:12Z

I was checking about the SparkLauncherSuite on Windows as vanzin's comment, and faced some trouble. It seems not to related with this PR, but I'm not sure yet. Please give me more time a little. When I resolve the problem, I'll rebase this PR.

tsudukim · 2015-04-27T10:20:48Z

The problem I mentioned was that the spark-shell.cmd which is called by SparkLauncherSuite somehow failed to launch test application.
It turned out to be caused by the limitation of Windows batch that the one command line must be shorter than 8192 characters. (The fullpath for classpath was long because I worked at a deep folder.)

So I assume all issue is now cleared up. Sorry for my late response.

SparkQA · 2015-04-27T13:57:49Z

Test build #719 has finished for PR 5227 at commit ac55787.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

vanzin · 2015-04-27T17:05:49Z

bq. limitation of Windows batch that the one command line must be shorter than 8192 characters

This may be fixable is it's something we really care about, but not related to this change.

Given that the test passes, LGTM (based on my previous comments - didn't look at the changes again).

SparkQA · 2015-04-27T18:20:42Z

Test build #31039 has started for PR 5227 at commit ac55787.

…path Modified to accept double-quotated args properly in spark-shell.cmd. Author: Masayoshi TSUZUKI <tsudukim@oss.nttdata.co.jp> Closes apache#5227 from tsudukim/feature/SPARK-6435-2 and squashes the following commits: ac55787 [Masayoshi TSUZUKI] removed unnecessary argument. 60789a7 [Masayoshi TSUZUKI] Merge branch 'master' of https://github.com/apache/spark into feature/SPARK-6435-2 1fee420 [Masayoshi TSUZUKI] fixed test code for escaping '='. 0d4dc41 [Masayoshi TSUZUKI] - escaped comman and semicolon in CommandBuilderUtils.java - added random string to the temporary filename - double-quotation followed by `cmd /c` did not worked properly - no need to escape `=` by `^` - if double-quoted string ended with `\` like classpath, the last `\` is parsed as the escape charactor and the closing `"` didn't work properly 2a332e5 [Masayoshi TSUZUKI] Merge branch 'master' into feature/SPARK-6435-2 04f4291 [Masayoshi TSUZUKI] [SPARK-6435] spark-shell --jars option does not add all jars to classpath

LK-Tmac1 · 2016-06-15T00:58:55Z

The same issue happens to the Mac OS. If multiple jar paths were specified, only the first one would be visible. Don't know if this issue was created and resolved or not.

[SPARK-6435] spark-shell --jars option does not add all jars to class…

04f4291

…path Modified to accept multiple jars in spark-shell.cmd.

tsudukim added 2 commits April 2, 2015 15:05

Merge branch 'master' into feature/SPARK-6435-2

2a332e5

Conflicts: bin/spark-class2.cmd

fixed test code for escaping '='.

1fee420

srowen reviewed Apr 2, 2015
View reviewed changes

tsudukim mentioned this pull request Apr 3, 2015

[SPARK-6568] spark-shell.cmd --jars option does not accept the jar that has space in its path #5347

Closed

vanzin reviewed Apr 3, 2015
View reviewed changes

Merge branch 'master' of https://github.com/apache/spark into feature…

60789a7

…/SPARK-6435-2 Conflicts: bin/spark-class2.cmd

removed unnecessary argument.

ac55787

asfgit closed this in 268c419 Apr 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

tsudukim commented Mar 27, 2015

SparkQA commented Mar 27, 2015

srowen commented Mar 27, 2015

vanzin commented Mar 27, 2015

vanzin commented Mar 27, 2015

srowen commented Mar 28, 2015

vanzin commented Mar 30, 2015

tsudukim commented Mar 31, 2015

tsudukim commented Mar 31, 2015

tsudukim commented Apr 2, 2015

SparkQA commented Apr 2, 2015

tsudukim commented Apr 2, 2015

srowen Apr 2, 2015

tsudukim Apr 2, 2015

srowen Apr 2, 2015

tsudukim Apr 2, 2015

srowen Apr 2, 2015

SparkQA commented Apr 2, 2015

vanzin Apr 3, 2015

tsudukim Apr 27, 2015

vanzin commented Apr 3, 2015

srowen commented Apr 6, 2015

srowen commented Apr 12, 2015

tsudukim commented Apr 13, 2015

srowen commented Apr 22, 2015

tsudukim commented Apr 24, 2015

tsudukim commented Apr 27, 2015

SparkQA commented Apr 27, 2015

vanzin commented Apr 27, 2015

SparkQA commented Apr 27, 2015

LK-Tmac1 commented Jun 15, 2016

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

[SPARK-6435] spark-shell --jars option does not add all jars to classpath #5227

Conversation

tsudukim commented Mar 27, 2015

SparkQA commented Mar 27, 2015

srowen commented Mar 27, 2015

vanzin commented Mar 27, 2015

vanzin commented Mar 27, 2015

srowen commented Mar 28, 2015

vanzin commented Mar 30, 2015

tsudukim commented Mar 31, 2015

tsudukim commented Mar 31, 2015

tsudukim commented Apr 2, 2015

SparkQA commented Apr 2, 2015

tsudukim commented Apr 2, 2015

srowen Apr 2, 2015

Choose a reason for hiding this comment

tsudukim Apr 2, 2015

Choose a reason for hiding this comment

srowen Apr 2, 2015

Choose a reason for hiding this comment

tsudukim Apr 2, 2015

Choose a reason for hiding this comment

srowen Apr 2, 2015

Choose a reason for hiding this comment

SparkQA commented Apr 2, 2015

vanzin Apr 3, 2015

Choose a reason for hiding this comment

tsudukim Apr 27, 2015

Choose a reason for hiding this comment

vanzin commented Apr 3, 2015

srowen commented Apr 6, 2015

srowen commented Apr 12, 2015

tsudukim commented Apr 13, 2015

srowen commented Apr 22, 2015

tsudukim commented Apr 24, 2015

tsudukim commented Apr 27, 2015

SparkQA commented Apr 27, 2015

vanzin commented Apr 27, 2015

SparkQA commented Apr 27, 2015

LK-Tmac1 commented Jun 15, 2016