[Spark] Make cancel() cancel the Spark jobs and stop only a session the runner created - #40103
[Spark] Make cancel() cancel the Spark jobs and stop only a session the runner created#40103tkaymak wants to merge 1 commit into
Conversation
131d037 to
159e6c0
Compare
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
…he runner created cancel() interrupted the execution thread and stopped the SparkSession from the caller thread. An interrupt does not cancel a Spark job, so with useActiveSparkSession a batch pipeline was never cancelled, and Builder.getOrCreate adopts a usable default session, so a pipeline could stop a session it shared with others. The execution thread kept working on a stopped SparkContext. The execution thread now runs under a job group, cancel() stops the evaluation, cancels the group and joins the thread. SparkSessionFactory counts the users of a session it created and stops it on the execution thread when the last one releases it. Batch EvaluationContext.stop() ends the leaf loop. A pipeline that ends after a cancel request reports CANCELLED. Fixes apache#40101.
159e6c0 to
71e7061
Compare
|
Assigning reviewers: R: @kennknowles added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
I understand this change tries to fix a few important gaps/bugs, however the behavior change part is likely undesiable. mainly cancel() becomes synchronous, and the reference counting may still not eliminate races of parallel jobs. The original diagnosis is spot-on:
In the Beam model, Reference counting: the choice of synchrnous Reference counting itself does not solve the race. It is the synchrnous Take a step back: it is a Spark limitation limiting us running jobs in parallel that would need different pipeline options won't have all configurations honored. This stems from the fact that only one active SparkContext is allowed throughout JVM. Need to think more about this. It's likely hard to find a proper fix. Would it possible to have a fix of miinimum behavior change (most notable the synchronous cancel) that could largely reduce the likelihood of race? |
| * limitations under the License. | ||
| */ | ||
| package org.apache.beam.runners.spark.structuredstreaming; | ||
|
|
There was a problem hiding this comment.
These tests may be useful, however, a generic idea is to keep test and main source balanced and test concise, as nowadays it's much easier to write boilerplate tests than before
| return SparkSession.active(); | ||
| } | ||
| return sessionBuilder(options.getSparkMaster(), options).getOrCreate(); | ||
| // Spark 3 also returns stopped sessions. |
There was a problem hiding this comment.
Please add a few comments noting why we need to manage active sessions ourselves now.
Fixes #40101. Found in review of #40090.
Bug
SparkStructuredStreamingPipelineResult.cancel()interrupted the execution thread withFuture.cancel(true)and then stopped the SparkSession from the caller thread. Neither is a cancellation. An interrupt does not cancel a Spark job,DAGScheduler.runJobwaits on theJobWaiteruntil it is cancelled explicitly, so withuseActiveSparkSession=truea batch pipeline was never cancelled andcancel()still reported CANCELLED.SparkSessionFactory.getOrCreateSessionusedBuilder.getOrCreate, which adopts a usable default session, so a pipeline could stop a session shared with other pipelines in the JVM. BatchEvaluationContext.stop()was a no-op. The execution thread kept translating on a stopped SparkContext, which is how the first Spark Versions run of #40090 failed.Fix, shared code compiled for Spark 3 and Spark 4
SparkSessionFactory.acquirerecords whether the runner created the session (no usable active or default session beforegetOrCreate) and counts users per created session.releasestops a created session when its last user leaves.useActiveSparkSession=truenever owns. Spark 3.5 returns stopped sessions fromgetActiveSessionandgetDefaultSession, so the usable check filters them the waygetOrCreatedoes.evaluate()when cancel arrived during translation, and releases the session infinally. The session stop therefore always happens after the last Spark call of the pipeline, on the thread that made it.EvaluationContext.stop()sets a flag the leaf loop checks. Running jobs are cancelled throughcancelJobGroupwithinterruptOnCancel.StreamingEvaluationContextkeeps stopping its queries.cancel()stops the context, cancels the job group, joins the execution thread without a timeout, and reports CANCELLED. The join is bounded by Spark itself, task interruption for batch andspark.sql.streaming.stopTimeoutfor queries. A pipeline that already ended reports DONE or FAILED. NoFuture.cancel(true), no terminal state callback.waitUntilFinishreports CANCELLED for a pipeline that ended after a cancel request.Tests, no sleeps
SparkStructuredStreamingPipelineResultTest: the join returns only after the execution ended, a secondcancel()is a no-op, a cancelled execution that ends with an exception is CANCELLED, not FAILED.StructuredStreamingPipelineStateTest: a batch cancel of a running job stops the job and the owned session; a pipeline on a session it did not create leaves that session running after cancel.Behavior changes to note, Spark 3 and Spark 4 structured streaming
cancel()blocks until the execution thread has ended. Tasks that ignore interruption delay it until Spark ends them,spark.task.reaper.enabledbounds that. Translation is not interrupted, a cancel during translation skips the evaluation.waitUntilFinish()after a cancel returns CANCELLED instead of throwing.Beam <jobName>.SparkSessionFactory.acquiredecides ownership under its own lock. A session created by other code on another thread between its check and itsgetOrCreatecall would be attributed to the runner, no Beam code path does that.SparkSessionFactory.getOrCreateSessionis replaced byacquireandrelease. The class is not annotated@Internal, the only caller in the repository was the runner.Gates: ErrorProne on native JDK 17, checkstyle, spotbugs, spotless,
:runners:spark:4:testfull,:runners:spark:3:testfor the touched classes, andStructuredStreamingPipelineStateTestten times in the CI fork mode.R: @Abacn