Abacn commented on PR #40103: URL: https://github.com/apache/beam/pull/40103#issuecomment-5640983590
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: 1. `cancel()` merely interrupting the execution thread (`Future.cancel(true)`) does not cancel running Spark jobs in the `DAGScheduler`. 2. Calling `session.stop()` synchronously from the caller thread in `offerNewState` tears down the `SparkContext` while the execution thread may still be actively translating or evaluating. In the Beam model, `PipelineResult.cancel()` is designed as an asynchronous cancellation request. Making it synchrounous can hang callers. In fact, synchrounous `cancel()` is what introduced most of the state-machine complexity in this PR (handling interrupted cancels, unobserved completions/failures, re-checking `pipelineExecution.isDone()`, etc.). Reference counting: the choice of synchrnous `cancel()` may be correlated with reference counting introduced. Previously we force cancel session on each job cancellation. Subsequent job then starts in a fresh new session thus its conf is honored. Now, if cancel remains async then subsequent job could run on same session and not honoring conf. Reference counting itself does not solve the race. It is the synchrnous `cancel()` force job submitting sequentially thus solving race, and parallel jobs happen to run on same session does not crash, but just have conf messed up, as a result less likely (but still possible) test failure. 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
