[SPARK-24785] [SHELL] Making sure REPL prints Spark UI info and then Welcome message #21749
Conversation
Update LICENSE Update JLine update scala-parser-combinators Error handling Fix compilation temp Revert hack remove hack
|
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.
|
Test build #92872 has finished for PR 21749 at commit
|
|
agreed, it's not ideal to use reflection... |
|
@skonto perhaps you folks can give some guidance on this? |
|
@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. |
|
Ping @zsxwing @gatorsmile |
|
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. |
dbtsai
changed the title
|
Test build #95052 has finished for PR 21749 at commit
|
This reverts commit 8715fbe.
dbtsai
changed the title
|
@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. |
|
Test build #95092 has finished for PR 21749 at commit
|
|
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. |
|
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. |
|
@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. |
|
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... |
|
@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