DanielLeens commented on PR #11077:
URL: https://github.com/apache/seatunnel/pull/11077#issuecomment-5461997373
Quick continuity check on the newest activity: since my last full review
(2026-08-29T00:21:33Z, head `e3e57c19518e`), exactly one commit has landed —
`8698a83d00b3` ("[Chore] Retry CI"). I checked it directly (`gh api
repos/apache/seatunnel/commits/8698a83d00b3...`): it changes 0 files. So there
is no new code on this PR to re-review; my previous pass already reflects the
current head's actual content byte-for-byte, and I re-verified that
independently by re-reading the full diff against `dev` again from scratch (all
of `MultiTableSink.java`, `MultiTableSinkWriter.java`, the committer classes,
`SeaTunnelSink.java`, and the file-sink changes) rather than trusting that
assumption.
Restating the standing conclusion so it's not lost in a long thread: **no
code blockers remain.** The two issues from my prior round (quarantine cascade
closing a shared writer out from under a healthy sibling table, and the legacy
merged-state restore leaking orphan tmp transactions for non-canonical UUID
prefixes) are both fixed and covered by targeted regression tests
(`testRuntimeFailureDoesNotCloseSharedWriterForHealthyAlias`,
`shouldCleanOrphanTransactionsForEveryRestoredUuidPrefix`) that I confirmed
actually exercise the failure scenario, not just the happy path. Two
non-blocking items remain open as documented carryovers:
- Medium: shared checkpoint/commit records are keyed by an
arbitrarily-chosen "canonical" alias (`aliasedIdentifiers.get(0)`); if that
specific source table is later removed from the job config while a sibling
alias to the same destination survives, the sibling can restore with empty
state on the next recovery because the checkpoint was never persisted under its
own identifier. Worth either documenting this constraint on
`getPhysicalDestinationIdentifier()` or keying shared records by a
destination-scoped synthetic identifier instead of an arbitrary alias.
- Low: `closeCreatedWriters` only triggers on `IOException` (an unchecked
exception from `createWriter`/`restoreWriter` would still leak already-created
sibling writers), and the resource-manager sizing in
`MultiTableSinkAggregatedCommitter` counts by alias rather than by distinct
writer instance.
CI update: `Build` is currently `in_progress` on this head rather than
failed, so the Maven Central 429 rate-limit flake I flagged last round on the
fork's run appears cleared by this retry. Will need to see it finish green.
One housekeeping item outside the code itself: `mergeStateStatus` is still
`BLOCKED` (`reviewDecision: REVIEW_REQUIRED`) because @SEZ9's
`CHANGES_REQUESTED` review is still standing from head `9a7bac1e4671`
(2026-08-23) — before either of the two fixes above landed. @SEZ9, could you
take a look at whether your two open items from that round are addressed on the
current head, or dismiss/update the review if they are? I can't do that myself
since I only have comment-level permissions here.
@hesam-oxe, no action needed from you right now beyond letting CI finish —
really solid work closing out the quarantine-cascade and orphan-transaction
issues with precise, minimal fixes and tests that reproduce the exact original
failure modes rather than just re-testing the happy path.
--
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]
