Uh oh!
There was an error while loading. Please reload this page.
[SPARK-24785] [SHELL] Making sure REPL prints Spark UI info and then Welcome message - #21749
[SPARK-24785] [SHELL] Making sure REPL prints Spark UI info and then Welcome message#21749dbtsai wants to merge 11 commits into
Conversation
Update LICENSE Update JLine update scala-parser-combinators Error handling Fix compilation temp Revert hack remove hack
SparkQA
commented
Jul 11, 2018
Test build #92871 has finished for PR 21749 at commit
|
| val field = classOf[ILoop].getDeclaredFields.filter(_.getName.contains("globalFuture")).head | ||
| field.setAccessible(true) | ||
| field.set(this, Future successful true) |
There was a problem hiding this comment.
This reflection has to be used to access private globalFuture in ILoop.
SparkQA
commented
Jul 11, 2018
Test build #92872 has finished for PR 21749 at commit
|
felixcheung
commented
Jul 12, 2018
agreed, it's not ideal to use reflection... |
felixcheung
commented
Jul 12, 2018
@skonto perhaps you folks can give some guidance on this? |
skonto
commented
Jul 12, 2018
@felixcheung sure I will add this to our https://docs.google.com/document/d/1fbkjEL878witxVQpOCbjlvOvadHtVjYXeB-2mgzDTvk where we track remaining issues. Scala team already is discussing this with @dbtsai. |
dbtsai
commented
Aug 20, 2018
Ping @zsxwing@gatorsmile |
dbtsai
commented
Aug 21, 2018
srowen
commented
Aug 21, 2018
I am fairly neutral on whether to hack to support multiple Scala 2.11 versions, and 2.11.12, or just leave it at 2.11.8. I think this hack is pretty low risk because I can't see any more 2.11.x releases coming, and won't rebreak. |
SparkQA
commented
Aug 22, 2018
Test build #95052 has finished for PR 21749 at commit
|
This reverts commit 8715fbe.
@srowen Scala 2.12 branch also has the wrong message printing order issue. I don't know if I have enough time to finish the work on the consolidation of Scala 2.11 and 2.12 codebase before we cut 2.4, so I will work on it in another PR. Can you help me to review this PR? Thanks. |
SparkQA
commented
Aug 22, 2018
Test build #95092 has finished for PR 21749 at commit
|
srowen
commented
Aug 22, 2018
As it's more a cosmetic issue than anything, and reverting the Scala 2.12 change at least I think will be a problem, I'd suggest not reverting the scala version, but instead fixing forward. (Or at least, fix 2.11 but don't revert the 2.12 version, and accept that it has an issue.) I'd love to unify the two Scala REPL source trees even if it costs some reflection. The change here looks OK on its surface; does it work? Did you open another PR where you fully merged the source trees? I swear I just saw it last night but can't find the other PR now, if so. That would really be the right fix, even if it has to wait until after 2.4, or even after the first RC. We still have Scala 2.12 issues in flight that I really want to fix for 2.4. |
gatorsmile
commented
Aug 22, 2018
Talked with @zsxwing . We do not need to revert the version bump, as long as this PR does not introduce a new regression. BTW, merging to the RC branches should be treated as backporting to the previous releases. |
dbtsai
commented
Aug 22, 2018
@srowen It does work, and I have used it for my development recently. You should be able to build with my branch to test it out. I was trying to merge two source trees yesterday, and I realized I need more time to carefully test out the classloader issues, so I decided to take it out from this PR. I have couple other tasks I want to deal with for 2.4 releases, and I will try to find time to work on it in RC period. @gatorsmile That will be great. We were using very old version of Scala 2.11, and it's a good time to upgrade it. |
vanzin
commented
Aug 22, 2018
The patch LGTM. I'd add a comment where Spark-specific code was inserted, so it's easy to find later. But ok as is too. Not sure why it seems like a big deal to have the Scala libraries add an initialization hook to that class... |
dbtsai
commented
Aug 22, 2018
@vanzin Thanks for the reviewing. I'll merge it as it, and have another PR trying to merge 2.11 and 2.12 branches. I had a positive conversation with Scala community to add a proper hook for us, and they agreed with it. |
What changes were proposed in this pull request?
After #21495 the welcome message is printed first, and then Scala prompt will be shown before the Spark UI info is printed.
Although it's a minor issue, but visually, it doesn't look as nice as the existing behavior. This PR intends to fix it by duplicating the Scala
processcode to arrange the printing order. However, one variable is private, so reflection has to be used which is not desirable.We can use this PR to brainstorm how to handle it properly and how Scala can change their APIs to fit our need.
How was this patch tested?
Existing test