DanielLeens commented on PR #11678:
URL: https://github.com/apache/seatunnel/pull/11678#issuecomment-5363682289
Thanks for the careful follow-up — happy to confirm both, and I re-verified
each one directly against the current head rather than just restating my
round-6 write-up.
**1. Fix 2 (`markSuccessfulAutoFlushBoundaryIfNeeded()` /
`releaseSavepointSilently()`) — confirmed resolved.**
I re-read `9f038035b`'s diff again just now. It hoists `Connection
connection = connectionProvider.getConnection();` once at the top of
`markSuccessfulAutoFlushBoundaryIfNeeded()`, uses that same local for
`setSavepoint()`, and passes it into `releaseSavepointSilently(connection,
previousSavepoint)`, which now takes `Connection` as a parameter instead of
calling `getConnection()` internally. That's a byte-for-byte match to what I
described as "after" in round 6 — both round-5 Medium findings (the
rollback/savepoint-mismatch risk and the doubled per-auto-flush validation
cost) are fixed in this single commit, and it's the only commit since round 5.
So yes: both blockers, not just Fix 1, are verified resolved on the current
head.
**2. `bb6d4dc` — confirmed already-merged content, no new PR-authored
logic.**
I diffed `bb6d4dc`'s version of
`MultiTableSinkWriterSchemaChangeBroadcastTest.java` against the current `dev`
tip and it's an exact match (empty diff) — and `dev`'s history shows that
content came from `#11726` ("Tolerate the pre-existing close() race in the
schema-change worker-failure test"), merged 2026-08-11. So `bb6d4dc` is purely
a cherry-pick of already-reviewed, already-merged upstream content onto this
branch; nothing new to review there.
**One more thing worth flagging while we're here:** `mergeStateStatus` is
still showing `BLOCKED`, and the reason is concrete, not stale caching — the
stray review posted under the `davidzollo` account is still recorded as
`CHANGES_REQUESTED` (not dismissed), so GitHub's review-decision logic still
counts it even though the authoritative `DanielLeens` copy was corrected to
`APPROVED` back in round 5 and reconfirmed in round 6. I only have read-level
review controls here, so I can't dismiss it myself — a maintainer with write
access on that review will need to dismiss `davidzollo`'s stray
`CHANGES_REQUESTED` entry before this can actually merge, independent of
anything code-related.
To summarize where this stands: no code changes needed from my side, CI was
fully green on `9f038035b` as of round 6, and the only remaining blocker to
merging is the stray review dismissal above. Thanks again for tracking this so
closely through every round.
--
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]
