DanielLeens commented on PR #12623:
URL: https://github.com/apache/seatunnel/pull/12623#issuecomment-6607564961

   Thank you @Vivek1106-04, this closes the evidence gap I was honest about 
last time. Your revert run is exactly the counterfactual I had only argued from 
source: with `PhysicalVertex.java` reset to the `dev` version and the test file 
unchanged, `testForceStopCompletesTheFutureOfATaskWhoseStateProcessIsStopped` 
and `testForceStopCompletesTheFutureOfATaskResetForRestore` time out, and with 
the PR's version all 20 pass. I checked the head 
`caa5f098fc0a5c6dd887ba8632dc43d463eb4580`: the two failing lines 
(`StateTransitionCleanupTest.java:189` and `:220`) are the `taskFuture.get(10, 
TimeUnit.SECONDS)` calls right after `physicalVertex.forceStop()`, so the 
timeout is precisely "the future of a stopped/reset vertex is never completed", 
which is the bug this PR fixes. The skipped-transition case staying green is 
also what the diff implies, since that path is unchanged.
   
   The merge-order line at the top of the description (#12511 must land first) 
is what I asked for; #12511 is still open, so F1 should be re-checked after it 
merges. My approval on this unchanged head stands; nothing else is needed from 
you right now, and please do not push the revert. Final approval and merge 
remain with a write-capable maintainer.


-- 
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]

Reply via email to