[SPARK-32029][SQL] Check spark context is stoped when get active session #28868
Conversation
ulysses-you
changed the title
|
Test build #124259 has finished for PR 28868 at commit
|
|
Test build #124263 has finished for PR 28868 at commit
|
|
Retest this please. |
|
Test build #124302 has finished for PR 28868 at commit
|
|
Test build #124312 has finished for PR 28868 at commit
|
|
close and check this offline. |
ulysses-you
changed the title
|
Test build #124779 has finished for PR 28868 at commit
|
| @@ -758,7 +758,7 @@ class SparkSession private( | |||
| // Use the active session thread local directly to make sure we get the session that is actually | |||
| // set and not the default session. This to prevent that we promote the default session to the | |||
| // active session once we are done. | |||
| val old = SparkSession.activeThreadSession.get() | |||
| val old = SparkSession.getActiveSession.get | |||
There was a problem hiding this comment.
unify the entrance of active session.
ulysses-you
changed the title
|
I don't think anything is meant to work after you shut down the SparkContext? |
|
Yes, it won't. But it would be best to do that if we not support it we should check and assert explicitly. |
|
Test build #124780 has finished for PR 28868 at commit
|
|
@srowen Does this pr make sense to you ? I'm ok if not necessary. |
| }.getMessage | ||
| assert(msg.contains("Cannot call methods on a stopped SparkContext.")) | ||
| val msg2 = intercept[IllegalStateException] { | ||
| SparkSession.getActiveSession |
There was a problem hiding this comment.
what's the behavior before this PR? is the session still use-able?
There was a problem hiding this comment.
Yes, now we only clear default session, active session is still alive.
| @@ -1020,7 +1020,9 @@ object SparkSession extends Logging { | |||
| // Return None when running on executors. | |||
| None | |||
| } else { | |||
| Option(activeThreadSession.get) | |||
| val session = Option(activeThreadSession.get) | |||
| session.foreach(_.sparkContext.assertNotStopped()) | |||
There was a problem hiding this comment.
I don't think it's necessary. A user may get the active session instance, and after that someone else stops the spark context. This check can't help this case.
There was a problem hiding this comment.
That's true. This place only checks if user get an active session, existing used session can not be covered. But in some case which used session.withActive() this can do the help.
What changes were proposed in this pull request?
Check spark context is stoped when get active session.
Why are the changes needed?
Now default session is set null when application end but active session still exists, we should forbid this.
Before this pr:
We can still use
spark.sql("show tables")aftersc.stop.Does this PR introduce any user-facing change?
BUG fix.
How was this patch tested?
Add UT.