[SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches #22246

dbtsai · 2018-08-27T17:57:00Z

What changes were proposed in this pull request?

Using some reflection tricks to merge Scala 2.11 and 2.12 codebase.

How was this patch tested?

Existing tests.

dbtsai · 2018-08-27T17:57:24Z

Ping @srowen

SparkQA · 2018-08-27T18:15:56Z

Test build #95302 has finished for PR 22246 at commit 075ca4a.

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

srowen · 2018-08-27T18:19:06Z

If it works, great! You can then remove the hacks in the POM to support 2.11 vs 2.12 code? I wonder if 2.12 will compile the other two classes found in the 2.11 tree.

dbtsai · 2018-08-27T18:29:59Z

@viirya The reflection trick we use in scala.reflect.internal.util.ScalaClassLoader doesn't work when the REPL is called from test. Do you have any idea about it? Thanks.

sbt.ForkMain$ForkError: java.lang.NoSuchMethodException: scala.reflect.internal.util.ScalaClassLoader$.savingContextLoader(scala.Function0)
	at java.lang.Class.getDeclaredMethod(Class.java:2130)
	at org.apache.spark.repl.SparkILoop.runClosure(SparkILoop.scala:156)
	at org.apache.spark.repl.SparkILoop.process(SparkILoop.scala:178)
	at org.apache.spark.repl.Main$.doMain(Main.scala:78)
	at org.apache.spark.repl.ReplSuite.runInterpreter(ReplSuite.scala:56)
	at org.apache.spark.repl.ReplSuite$$anonfun$11.apply(ReplSuite.scala:141)

@srowen The other two files in 2.11 can not be compiled in 2.12 . I removed the POM hack as well. Thanks for pointing out.

srowen · 2018-08-27T18:33:49Z

Oh, but if there's on tree for both versions, and it has files that 2.11 needs but 2.12 can't compile, won't it fail? Not sure about the NSME error up there, I haven't seen it.

dbtsai · 2018-08-27T19:53:21Z

SparkILoopInterpreter.scala and SparkExprTyper.scala are only on 2.11 branch. It can not be compiled in 2.12. I think it's okay to have them in 2.11 branch since they are not needed in 2.12. We can remove those two files when the support of 2.11 is ended.

[error] /Users/d_tsai/dev/clean/apache-spark/repl/src/main/scala/org/apache/spark/repl/SparkILoopInterpreter.scala:80: overriding lazy value importableSymbolsWithRenames in class ImportHandler of type List[(this.intp.global.Symbol, this.intp.global.Name)];
[error]  lazy value importableSymbolsWithRenames needs `override' modifier
[error]       lazy val importableSymbolsWithRenames: List[(Symbol, Name)] = {
[error]                ^
[warn] two warnings found
[error] one error found
[error] Compile failed at Aug 27, 2018 11:51:19 AM [0.613s]

srowen · 2018-08-27T19:59:31Z

I understand, you still need the extra source files in a separate tree just for 2.11.

dbtsai · 2018-08-27T20:02:36Z

Yes. But we can remove the code in 2.12, and this can also fix the printing order issue in 2.12.

I don't know why we get some reflection error in tests. It works in command line. I will do some debugging tonight.

dbtsai · 2018-08-27T20:39:26Z

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

+    if (isScala2_11) {
+      val loader = Utils.classForName("scala.reflect.internal.util.ScalaClassLoader$")
+        .getDeclaredField("MODULE$")
+        .get(null)


@viirya Out of my curiosity, since it's a static method, in theory, in the invoke, the first arg should be null. Why do you put the loader in? Also, this doesn't work in tests, and we get the method not found exception.

Although it is a static method in Scala, it will be compiled to a non-static method in Java class. This class has a static member of the class type. The access of all Scala static methods are through the static member.

SparkQA · 2018-08-27T22:31:59Z

Test build #95304 has finished for PR 22246 at commit 6203f83.

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

viirya · 2018-08-28T03:13:35Z

@viirya The reflection trick we use in scala.reflect.internal.util.ScalaClassLoader doesn't work when the REPL is called from test. Do you have any idea about it? Thanks.

Yeah, it seems due to classloader. After changed to Spark classloader, the tests were passed locally. Let's see if Jenkins tests pass too.

SparkQA · 2018-08-28T07:05:01Z

Test build #95324 has finished for PR 22246 at commit e0d424d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-08-28T07:10:42Z

retest this please.

SparkQA · 2018-08-28T11:01:35Z

Test build #95333 has finished for PR 22246 at commit e0d424d.

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

viirya · 2018-08-28T11:47:21Z

retest this please.

SparkQA · 2018-08-28T15:51:27Z

Test build #95348 has finished for PR 22246 at commit e0d424d.

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

kiszk · 2018-08-28T16:02:04Z

retest this please

dbtsai · 2018-08-28T18:44:00Z

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

+        settings.classpath append addedClasspath
+      }
+      intp = Utils.classForNameFromSpark("org.apache.spark.repl.SparkILoopInterpreter")
+        .getDeclaredConstructor(Seq(classOf[Settings], classOf[JPrintWriter]): _*)


To summarize the classLoader issue here, classOf[Settings] will use the classloader of the object of SparkILoop, and Utils.classForName will use the classloader of the current running thread which can lead to mismatching.

I think using Utils.classForNameFromSpark is dangerous since there is no guaranteed that this will use the classloader of this object which is used in classOf[Settings].

I will use Class.ofName to ensure the consistency.

Good point!

SparkQA · 2018-08-28T20:52:23Z

Test build #95360 has finished for PR 22246 at commit e0d424d.

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

dbtsai · 2018-08-28T21:05:28Z

@srowen @kiszk it works now. Can you have a pass on this PR? Thanks.

SparkQA · 2018-08-29T01:51:24Z

Test build #95383 has finished for PR 22246 at commit b147dba.

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

SparkQA · 2018-08-29T02:01:41Z

Test build #95381 has finished for PR 22246 at commit af4d984.

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

viirya · 2018-08-29T02:10:12Z

LGTM

SparkQA · 2018-08-29T03:51:06Z

Test build #95393 has finished for PR 22246 at commit bab5947.

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

dbtsai · 2018-08-29T04:31:17Z

Thanks all for reviewing. Merged into master.

dbtsai and others added 3 commits August 27, 2018 00:10

Consolidated Scala 2.11 and 2.12 branches

8669c21

Get static loader object and invoke method on it.

3808f02

Changed message

075ca4a

remove 2.12 pom hack

6203f83

dbtsai changed the title ~~[SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches~~ [WIP] [SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches Aug 27, 2018

dbtsai commented Aug 27, 2018

View reviewed changes

Use Spark classloader.

e0d424d

dbtsai changed the title ~~[WIP] [SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches~~ [SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches Aug 28, 2018

dbtsai commented Aug 28, 2018

View reviewed changes

Updated

af4d984

dbtsai added 2 commits August 28, 2018 14:07

Remove unused import

b147dba

Reduce one reflection

bab5947

srowen approved these changes Aug 29, 2018

View reviewed changes

asfgit closed this in ff8dcc1 Aug 29, 2018

[SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches #22246

[SPARK-25235] [SHELL] Merge the REPL code in Scala 2.11 and 2.12 branches #22246

Uh oh!

Conversation

dbtsai commented Aug 27, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dbtsai commented Aug 27, 2018

Uh oh!

SparkQA commented Aug 27, 2018

Uh oh!

srowen commented Aug 27, 2018

Uh oh!

dbtsai commented Aug 27, 2018

Uh oh!

srowen commented Aug 27, 2018

Uh oh!

dbtsai commented Aug 27, 2018

Uh oh!

srowen commented Aug 27, 2018

Uh oh!

dbtsai commented Aug 27, 2018

Uh oh!

dbtsai Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 27, 2018

Uh oh!

viirya commented Aug 28, 2018

Uh oh!

SparkQA commented Aug 28, 2018

Uh oh!

viirya commented Aug 28, 2018

Uh oh!

SparkQA commented Aug 28, 2018

Uh oh!

viirya commented Aug 28, 2018

Uh oh!

SparkQA commented Aug 28, 2018

Uh oh!

kiszk commented Aug 28, 2018

Uh oh!

dbtsai Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 28, 2018

Uh oh!

dbtsai commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Aug 29, 2018

Uh oh!

SparkQA commented Aug 29, 2018

Uh oh!

viirya commented Aug 29, 2018

Uh oh!

SparkQA commented Aug 29, 2018

Uh oh!

dbtsai commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya Aug 28, 2018 •

edited

Loading

dbtsai commented Aug 28, 2018 •

edited

Loading