DanielLeens commented on PR #11924:
URL: https://github.com/apache/seatunnel/pull/11924#issuecomment-5522174791
Thanks for jumping in and clarifying the ownership on #11999, @BinTaoMa —
that's helpful context for the maintainers deciding priority here.
From a pure code-review standpoint on **this** PR (#11924), the coordination
question doesn't change my open finding. The head is still `4a5e69f1c08a`,
unchanged since my last re-review on 2026-08-31/09-01 (the only commit since
then just merged `dev` to resolve conflicts and doesn't touch
`TaskExecutionService.java`). So Issue 1 from my 2026-08-25 review is still
unresolved at the current head — I just re-pulled the file at this exact SHA to
confirm:
- `TaskExecutionService.java:1184-1185` — `if (groupFutures != null &&
groupFutures.isEmpty()) { timerFlushFutures.remove(taskGroupLocation,
groupFutures); }`
- `TaskExecutionService.java:1211-1212` — same `groupFutures.isEmpty()` →
`timerFlushFutures.remove(...)` pattern inside
`closeTimerFlushTaskForGeneration`
- `TaskExecutionService.java:1215-1216` — `if (generationFutures.isEmpty())
{ timerFlushFuturesByGeneration.remove(executionGeneration, generationFutures);
}`
All three are still non-atomic `isEmpty()`-then-`remove(key, map)` checks: a
new timer-flush future can be registered into the same
`groupFutures`/`generationFutures` map in the gap between the `isEmpty()` read
and the `remove()` call, and that entry then gets silently dropped from the
outer map even though it's non-empty at removal time — the same race @dybyte
flagged and you already fixed correctly for `cancelAsyncFunction` via
`computeIfPresent(...)`. That fix just hasn't been mirrored to these three
timer-flush spots yet. This is still High severity/blocking for #11924
regardless of how #11755 priority is resolved between the two PRs.
On the #11999-vs-#11924 sequencing question itself: that's a scope/priority
call for the maintainers rather than something I can settle from a diff review
— I haven't reviewed #11999's implementation, so I can't vouch for its
correctness, only note (per your and @JeremyXin's description) that it takes a
narrower `synchronized`-gate approach without the generation-scoped future
registries, so it wouldn't inherit this PR's still-open Issue 1. If the
maintainers do land #11999 first as the near-term fix, I'd still want #11924 to
close out Issue 1 (and ideally land the regression coverage its own
`testTimerFlushCloseOnlyCancelsOwnedExecutionGeneration` test is targeting)
before merge, whether as a fresh PR or a rebase on top of #11999.
Separately, FYI on CI: the current head's `Build` check on the apache side
shows `cancelled` (https://github.com/apache/seatunnel/runs/99465692849, ran
~9h then got cancelled with no steps recorded), and there's no commit status
ever posted for `4a5e69f1c08a` (`state: pending`, 0 statuses) — so this head
hasn't actually completed a CI run yet, independent of the code review outcome
above.
I'll do a fresh full re-review as soon as Issue 1 is addressed and/or the
coordination with #11999 is resolved.
--
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]
