DanielLeens commented on PR #10680:
URL: https://github.com/apache/seatunnel/pull/10680#issuecomment-5412582707
Hi @SEZ9 and @xyueji, before going further with the rebase plan I want to
flag something that changes the picture here.
I independently re-checked the merge conflict just now with `git merge-tree`
against the current `dev` HEAD, and confirmed it live via the GitHub API
(`mergeable: CONFLICTING`, `mergeStateStatus: DIRTY`). The conflict is entirely
inside the same two files this PR touches: `StarRocksSinkManager.java` and
`StarRocksStreamLoadVisitor.java`.
Tracing why: `dev` already contains commit `46fefa7c61` —
**[Fix][Connector-V2] Prevent silent StarRocks batch data loss (#11431)**,
merged 2026-07-21, authored by @davidzollo. Its own PR description says
explicitly: *"Related to #10680. Thanks to @xyueji for identifying the
label-reuse failure mode."* That patch:
- retains the pending rows and Stream Load label across failed `flush()`
calls instead of clearing the batch,
- queries the existing label's state before deciding to finish or retry,
- creates a replacement label only when StarRocks reports `ABORTED`,
- adds a regression matrix covering
`VISIBLE`/`COMMITTED`/`ABORTED`/`UNKNOWN`/retry-exhaustion/interruption for
exactly this scenario.
That is functionally the same fix this PR's own commit `0570563e5b`
describes ("checks the existing StarRocks label state before deciding whether
to finish or retry, only creates a new label after an ABORTED transaction,
preserves the batch for UNKNOWN/errors/interruption"), and it's why the
conflict lands in exactly these two methods — `dev` didn't drift incidentally,
it independently solved the same problem this branch is solving.
So the concrete question before anyone spends effort on a 657-commit rebase:
**is there anything in this branch's approach that `dev`'s current
`StarRocksSinkManager`/`StarRocksStreamLoadVisitor` (post-#11431) doesn't
already cover?** If not, the right outcome is closing this PR as superseded by
#11431 rather than carrying the rebase/CI-retrigger/approval sequence forward
for a fix that already shipped. If there is a real gap #11431 missed, that's
worth stating explicitly so the remaining work is scoped to the delta, not a
full resync.
@xyueji, could you take a look at the current `dev` implementation of those
two files and confirm either way? That's a faster path to closure than the
rebase-then-CI-then-approval sequence discussed above, and avoids duplicating
test coverage that already exists on `dev`.
--
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]
