[SPARK-7944][SPARK-8013] Remove most of the Spark REPL fork for Scala 2.11 #6903

dragos · 2015-06-19T13:53:41Z

This PR removes most of the code in the Spark REPL for Scala 2.11 and leaves just a couple of overridden methods in SparkILoop in order to:

change welcome message
restrict available commands (like :power)
initialize Spark context

The two codebases have diverged and it's extremely hard to backport fixes from the upstream REPL. This somewhat radical step is absolutely necessary in order to fix other REPL tickets (like SPARK-8013 - Hive Thrift server for 2.11). BTW, the Scala REPL has fixed the serialization-unfriendly wrappers thanks to @ScrapCodes's work in #4522

All tests pass and I tried the spark-shell on our Mesos cluster with some simple jobs (including with additional jars), everything looked good.

As soon as Scala 2.11.7 is out we need to upgrade and get a shaded jline dependency, clearing the way for SPARK-8013.

/cc @pwendell

Make sure the class path is correctly passed to the underlying REPL.

srowen · 2015-06-19T15:02:05Z

Sounds like a good step to avoid future headaches, and would surely be great to avoid continuing to fork. CC @mateiz as well to recall any particular things that were important to do differently in the fork, to help make sure nothing is being overlooked? Of course this is just for 2.11.

dragos · 2015-06-19T15:25:56Z

See scala/scala#4563 for shaded jline.

SparkQA · 2015-06-19T15:47:46Z

Test build #35268 has finished for PR 6903 at commit 9d46d85.

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

ScrapCodes · 2015-06-21T08:12:47Z

repl/scala-2.11/src/main/scala/org/apache/spark/repl/SparkILoop.scala

-    else
-      intp.lastWarnings foreach { case (pos, msg) => intp.reporter.warning(pos, msg) }
-  }
+  private val blockedCommands = Set("implicits", "javap", "power", "type", "kind")


Why did you block these commands, may be to retain the original behavior or something ?

Yes, I just followed the original behaviour.

The reason for blocking them earlier was that they were non functional or it needed a more complex change to support them. May be that is not the case now, or even if we want to retain the original behavior we can hide them behind a flag like -Dspark.repl.experiemental.commands or something. If you think these commands work as expected with spark.

I don't see any reason they wouldn't (I'll make sure), but I would add them in a separate pull request, so we can point to a JIRA in release notes. What do you think?

Sounds good.

ScrapCodes · 2015-06-21T08:16:33Z

Thanks for putting this together, I am going to take a more closer look. Finally we will be relieved of burden of maintaining and porting repl per release.

dragos · 2015-06-21T10:23:08Z

@ScrapCodes thanks for having a look!

mateiz · 2015-06-21T21:11:31Z

Hey Dragos, so what Scala 2.11 versions will this work with? It won't work with the very first one, right, only with versions after some of your other fixes? We need to document that so that people running older Scala 2.11s won't be confused.

dragos · 2015-06-22T09:17:16Z

@mateiz Indeed. I tested this with Scala 2.11.6, but I would prefer to move to 2.11.7 (should be out this week) ASAP. 2.11.7 has a shaded version of jline, which means we can build hive-thriftserver and spark-sql would work too.

Currently, Spark depends on Scala 2.11.6, and that's the version that gets bundled in the assembly jar (and the distribution). I didn't find any mention in the docs on how to build with a different Scala version, so my assumption was that Spark fixes the Scala version you're going to use in the spark-shell.

(I'm pretty sure a determined dev would find a way to pass scala.version from the outside, but it doesn't seem to be a supported scenario).

mateiz · 2015-06-22T18:26:17Z

Alright, please also send a PR to update the version to 2.11.7 when that comes out the. Both these things should be able to make it into Spark 1.5.

ScrapCodes · 2015-06-23T06:04:48Z

We can change the License file too, and add this too RAT checks.

ScrapCodes · 2015-06-23T06:05:41Z

repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala

@@ -274,7 +268,7 @@ class ReplSuite extends SparkFunSuite {

  test("SPARK-2632 importing a method from non serializable class and not using it.") {
    val output = runInterpreter("local",
-    """
+      """


Probably a stray change.

Actually no, arguments should be indented 2 spaces, right?

ScrapCodes · 2015-06-23T06:08:54Z

IMHO this can not be merged before 2.11.7 is released.

dragos · 2015-06-23T15:56:35Z

@ScrapCodes, @mateiz Let me know if there's anything else.

SparkQA · 2015-06-23T17:49:27Z

Test build #35557 has finished for PR 6903 at commit 66eec11.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dragos · 2015-06-23T20:39:39Z

I think this is a spurious failure:

[error] Failed: Total 1409, Failed 1, Errors 0, Passed 1408, Ignored 5
[error] Failed tests:
[error]     org.apache.spark.network.nio.ConnectionManagerSuite

SparkQA · 2015-06-24T09:57:39Z

Test build #957 has finished for PR 6903 at commit 66eec11.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dragos · 2015-06-24T12:11:45Z

Still spurious? This time it's a PySpark test.

SparkQA · 2015-06-24T14:45:04Z

Test build #958 has finished for PR 6903 at commit 66eec11.

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

ScrapCodes · 2015-06-25T04:59:36Z

Can you update to scala 2.11.7 as part of this ? It is just a flip in version numbers in pom.xml

ScrapCodes · 2015-06-25T04:59:49Z

BTW http://scala-lang.org/news/2.11.7

dragos · 2015-06-25T10:48:41Z

Yes, but it's a bit more than that. I'd like to make sure thirftserver also builds, and that requires some jline version tinkering as well (it needs to pull in the one from the Hive dependency now).

rat-excludes and removed `-Yrepl-sync` per reviewer’s request.

dragos · 2015-06-25T12:21:43Z

We are in fork-hell. Who can build and publish (or at least locate this)?

org.spark-project#genjavadoc-plugin_2.11.7;0.9-spark0: not found

ScrapCodes · 2015-06-25T13:07:03Z

I am sure @pwendell or @andrewor14 can help.

mengxr · 2015-07-07T14:42:32Z

@dragos @srowen Sorry I missed this thread! The official genjavadoc doesn't support 2.11.7 yet (https://github.com/typesafehub/genjavadoc/blob/master/project/Build.scala#L20). I can try to publish one for Scala 2.11.7 (if it compiles under Scala 2.11.7).

dragos · 2015-07-07T14:53:49Z

@mengxr I think (0 to 7) is inclusive, so probably it would work. Thanks! Looking forward to closing this one :)

brkyvz · 2015-07-07T15:33:17Z

Hey, sorry for missing this thread. I'll publish the 2.11.7 version today and ping you all.

mengxr · 2015-07-07T17:13:02Z

@dragos Sorry I was looking at my local master. They added 2.11.7 12 days ago:)

brkyvz · 2015-07-07T17:13:58Z

@dragos Just published the genjavadoc 2.11.7!

brkyvz · 2015-07-07T17:14:55Z

repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala


 import org.apache.commons.lang3.StringEscapeUtils
-import org.apache.spark.{SparkContext, SparkFunSuite}
+import org.apache.spark.{ SparkContext, SparkFunSuite }


could you remove spaces here.

dragos · 2015-07-08T08:44:35Z

Thanks @brkyvz. In principle there's nothing I need to do for this PR (except the whitespace changes you pointed out), but I'll run tests locally again.

Could you point to the source of your fork? This would make things easier in the future.

SparkQA · 2015-07-08T11:59:01Z

Test build #1006 has finished for PR 6903 at commit 0ce67a6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dragos · 2015-07-08T12:19:19Z

I removed spaces around imports (can't this be checked by ScalaStyle?).

I noticed a few tests are looping on 2.11 (both on 2.11.6 and 2.11.7). I'll investigate them, but I don't think they have anything to do with these changes.

ScrapCodes · 2015-07-08T12:28:42Z

Yes, it can be. But enforcing it codebase wide is huge(+ unjustified, in the sense it affect code history) effort.
See : https://github.com/scalastyle/scalastyle/blob/master/src/main/scala/org/scalastyle/scalariform/SpaceAroundTokenChecker.scala

I think, may be we should not block on those tests, but would be good to open individual issues for everything we sort of postponed for a time later ?

SparkQA · 2015-07-08T13:08:15Z

Test build #1008 has finished for PR 6903 at commit 0ce67a6.

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

srowen · 2015-07-08T13:34:24Z

I think this is a legit failure?

Scalastyle checks failed at following occurrences:
[error] sbt.IncompatiblePluginsException: Binary incompatibility in plugins detected.
[error] Note that conflicts were resolved for some dependencies:
[error]     org.codehaus.plexus:plexus-utils
[error]     org.apache.maven.wagon:wagon-provider-api
[error]     org.sonatype.sisu:sisu-inject-plexus
[error]     org.sonatype.sisu:sisu-inject-bean
[error]     org.sonatype.sisu:sisu-guice
[error]     cglib:cglib
[error]     asm:asm
[error]     com.typesafe:config
[error]     commons-lang:commons-lang
[error]     com.thoughtworks.paranamer:paranamer
[error] Use 'last' for the full log.
[error] running /home/jenkins/workspace/NewSparkPullRequestBuilder/dev/lint-scala ; received return code 1

dragos · 2015-07-08T13:38:23Z

Looks like that... I have no idea what it means though. I'll have a look

dragos · 2015-07-08T15:07:51Z

I can't reproduce it. How did you run it?

 $ dev/lint-scala 
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
Scalastyle checks passed.

(Apache RAT takes an hour on my system, no idea what file trips it so badly, so I just ran lint tests).

SparkQA · 2015-07-08T15:08:40Z

Test build #36793 has finished for PR 6903 at commit 2b1a305.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2015-07-08T15:18:17Z

Have a look at http://stackoverflow.com/questions/22628547/binary-incompatibility-in-plugins-detected-in-play-2-1-3-project-after-upgrading which makes me think this is somehow related to jline. Why you don't reproduce it, I don't know; does it depend on the profile that's set? I have no idea about jline but I imagine you do; does that ring any bells?

dragos · 2015-07-08T15:46:38Z

But the latest Jenkins build succeeded (at least, style checks did). Now it fails a test in catalyst. So this is a spurious error, it seems?

Note that there is no jline in the Jenkins log. That happened to be the case in the stack overflow question, but I doubt it's the same root cause.

Since I can't reproduce it, I also can't run last to see what was the full error message....

SparkQA · 2015-07-08T17:42:12Z

Test build #1012 has finished for PR 6903 at commit 2b1a305.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2015-07-08T17:52:59Z

Yeah these are different failures each time. I'm not sure what's going on -- still am not sure why these changes would make things merely flakier. Let me spin the wheel again.

squito · 2015-07-08T19:28:08Z

I've seen that failure randomly as well from changes that didn't change anything remotely related. It doesn't mean that the error is always spurious, but I wouldn't worry too much if it passes later on ...

SparkQA · 2015-07-08T19:50:50Z

Test build #1014 has finished for PR 6903 at commit 2b1a305.

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

dragos · 2015-07-09T08:32:54Z

Spinning the wheel "fixed" it :)

srowen · 2015-07-09T09:51:36Z

I think this is good to go. Any last comments anyone?

srowen · 2015-07-10T10:42:05Z

@dragos ah, sorry we seem to have merge conflicts now. I think we can merge after that's resolved and re-tested.

dragos · 2015-07-10T11:23:04Z

Oh, that's great...

SparkQA · 2015-07-10T15:11:02Z

Test build #37034 has finished for PR 6903 at commit c596c6f.

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

dragos added 2 commits June 19, 2015 14:59

Remove the REPL forked code.

abcc7cb

Fix SPARK-7944.

9d46d85

Make sure the class path is correctly passed to the underlying REPL.

ScrapCodes reviewed Jun 21, 2015
View reviewed changes

ScrapCodes reviewed Jun 23, 2015
View reviewed changes

Rename package and add license on top of the file, remove files from

b300183

rat-excludes and removed `-Yrepl-sync` per reviewer’s request.

dragos added 2 commits June 25, 2015 15:58

Change jline dependency and bump Scala version.

2b15962

Merge branch 'master' into issue/no-spark-repl-fork

b541930

brkyvz reviewed Jul 7, 2015
View reviewed changes

Removed spaces around multiple imports.

2b1a305

Merge branch 'master' into issue/no-spark-repl-fork

c596c6f

asfgit closed this in 11e22b7 Jul 10, 2015

PanAeon mentioned this pull request Dec 24, 2015

Serialization/classloading issues on spark 1.5 and scala 2.11 spark-notebook/spark-notebook#481

Closed

[SPARK-7944][SPARK-8013] Remove most of the Spark REPL fork for Scala 2.11 #6903

[SPARK-7944][SPARK-8013] Remove most of the Spark REPL fork for Scala 2.11 #6903

Conversation

dragos commented Jun 19, 2015

srowen commented Jun 19, 2015

dragos commented Jun 19, 2015

SparkQA commented Jun 19, 2015

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

ScrapCodes commented Jun 21, 2015

dragos commented Jun 21, 2015

mateiz commented Jun 21, 2015

dragos commented Jun 22, 2015

mateiz commented Jun 22, 2015

ScrapCodes commented Jun 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScrapCodes commented Jun 23, 2015

dragos commented Jun 23, 2015

SparkQA commented Jun 23, 2015

dragos commented Jun 23, 2015

SparkQA commented Jun 24, 2015

dragos commented Jun 24, 2015

SparkQA commented Jun 24, 2015

ScrapCodes commented Jun 25, 2015

ScrapCodes commented Jun 25, 2015

dragos commented Jun 25, 2015

dragos commented Jun 25, 2015

ScrapCodes commented Jun 25, 2015

mengxr commented Jul 7, 2015

dragos commented Jul 7, 2015

brkyvz commented Jul 7, 2015

mengxr commented Jul 7, 2015

brkyvz commented Jul 7, 2015

Choose a reason for hiding this comment

dragos commented Jul 8, 2015

SparkQA commented Jul 8, 2015

dragos commented Jul 8, 2015

ScrapCodes commented Jul 8, 2015

SparkQA commented Jul 8, 2015

srowen commented Jul 8, 2015

dragos commented Jul 8, 2015

dragos commented Jul 8, 2015

SparkQA commented Jul 8, 2015

srowen commented Jul 8, 2015

dragos commented Jul 8, 2015

SparkQA commented Jul 8, 2015

srowen commented Jul 8, 2015

squito commented Jul 8, 2015

SparkQA commented Jul 8, 2015

dragos commented Jul 9, 2015

srowen commented Jul 9, 2015

srowen commented Jul 10, 2015

dragos commented Jul 10, 2015

SparkQA commented Jul 10, 2015