davidzollo commented on PR #11856:
URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5426486083
Pushed the fix for the race in `94eaf415`, going with **Option A** from the
discussion above so @SEZ9 doesn't need to build the follow-up commit.
What changed:
- `PhysicalVertex` now records the failure message and the
coordinator-classified graceful member-removal flag as one immutable
`FailureClassification` holder, installed by a single first-write-wins
`compareAndSet` — the mismatched `compareAndSet`/`set` pair on two independent
atomics is gone. Both writer paths (`updateStateByExecutionService` and
`makeTaskGroupFailing`) go through the same install method, and the FAILED
branch of `stateProcess()` reads the holder once, so the message and its
classification can never be observed as a torn pair.
- One deliberate nuance to preserve the pre-existing semantics exactly: a
`null` message never claims the slot (the old `compareAndSet(null, null)`
"succeeded" but left the reference empty, so a later real message could still
win it); an empty-string message still claims the slot, as before. The
graceful=true path always carries a real message
(`buildMemberRemovedFailureState` wraps `JobException`), so WARN classification
for genuine graceful removals is unaffected, and every degenerate path falls
toward ERROR — the safe direction.
Tests added in `PhysicalVertexTest`:
- `shouldNotRetagRecordedGenuineFailureAsGraceful` — deterministically
reproduces the exact interleaving from the review (genuine failure records
first, node-offline classification lands after): the recorded pair must stay
`{genuine message, graceful=false}`. On the previous head this same sequence
ended with `{genuine message, graceful=true}`, i.e. the WARN misclassification.
- `shouldNotStripGracefulFlagFromRecordedOfflineFailure` — the reverse order
stays `{offline message, graceful=true}`, matching first-write-wins.
- `shouldIgnoreNullMessageAndKeepSlotClaimable` — the null-message semantics
above.
- `shouldKeepMessageAndFlagPairedUnderConcurrentWriters` — two
latch-released writer threads, 100 iterations, order-independent assertion: the
recorded flag always belongs to whichever message won the slot.
Verification: ran module-scoped `spotless:apply`/`spotless:check` locally
(clean); compile/test runs on GitHub CI against this head (`94eaf415`). The
previous run's only failure was `RocketMqIT.testSourceRocketMqRestore` in
`rocketmq-connector-it` (message-count flake preceded by a
`RemotingSendRequestException` against the test container) — unrelated to this
engine-only diff. I'll track the new run and retry at job granularity if only
that flake reappears.
@SEZ9 with this, the 8 findings from your `CHANGES_REQUESTED` review and the
race flagged afterwards are all addressed on the current head — please take a
fresh look when you get a chance.
--
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]
