[SPARK-59242][SQL] Guard the DAGScheduler cleanup in SQLExecution against a stopped SparkContext - #58518
Open
ivoson wants to merge 1 commit into
Open
[SPARK-59242][SQL] Guard the DAGScheduler cleanup in SQLExecution against a stopped SparkContext#58518ivoson wants to merge 1 commit into
ivoson wants to merge 1 commit into
Conversation
…inst a stopped SparkContext ### What changes were proposed in this pull request? Guards the `sc.dagScheduler` dereferences in `SQLExecution.withNewExecutionId0`'s `finally` block so a query unwinding while the `SparkContext` is being torn down no longer throws a `NullPointerException` that destroys the query's real failure. `SparkContext.stop()` nulls `_dagScheduler` before it stops the listener bus, so a query that ends during teardown can reach `sc.dagScheduler.cleanupQueryJobs(executionId)` with a null receiver. Because that call runs in a `finally`, the NPE *replaces* the query's real exception rather than being suppressed, and it skips the rest of the block: the `SparkListenerSQLExecutionEnd` event is never posted, and `observationManager.tryComplete` is never called (leaving any `Observation.get` waiter blocked). The `activeQueryToJobs` dereference under `Utils.isTesting` sits earlier in the same `finally` and hits the same race, so it is guarded as well; without that a test cannot reach the `cleanupQueryJobs` call site. ### Why are the changes needed? A query failing (or completing) exactly as its `SparkContext` is being stopped surfaces as an unclassified internal `NullPointerException` from `SQLExecution`, hiding the real error and leaving observation waiters hung, instead of propagating the true failure. ### Does this PR introduce any user-facing change? No. It only prevents an NPE from masking the real query outcome during context teardown. ### How was this patch tested? New unit tests in `SQLExecutionSuite` that null out the `DAGScheduler` (standing in for a stopped `SparkContext`, which nulls `_dagScheduler` before stopping the listener bus) and assert that `withNewExecutionId` surfaces the body's real failure rather than an NPE, and completes normally on the success path. ### Was this patch authored or co-authored using generative AI tooling? Yes, ported from an internal Databricks Runtime fix using Claude Code (Opus 4.8). Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Guards the
sc.dagSchedulerdereferences inSQLExecution.withNewExecutionId0'sfinallyblock so a query unwinding while theSparkContextis being torn down no longer throws aNullPointerExceptionthat destroys the query's real failure.SparkContext.stop()nulls_dagSchedulerbefore it stops the listener bus, so a query that ends during teardown can reachsc.dagScheduler.cleanupQueryJobs(executionId)with a null receiver. Because that call runs in afinally, the NPE replaces the query's real exception rather than being suppressed, and it skips the rest of the block: theSparkListenerSQLExecutionEndevent is never posted, andobservationManager.tryCompleteis never called (leaving anyObservation.getwaiter blocked).The
activeQueryToJobsdereference underUtils.isTestingsits earlier in the samefinallyand hits the same race, so it is guarded as well; without that a test cannot reach thecleanupQueryJobscall site.Why are the changes needed?
A query failing (or completing) exactly as its
SparkContextis being stopped surfaces as an unclassified internalNullPointerExceptionfromSQLExecution, hiding the real error and leaving observation waiters hung, instead of propagating the true failure.Does this PR introduce any user-facing change?
No.
How was this patch tested?
UTs added.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)