SEZ9 commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5225133898
Thanks @nzw921rx for the thorough deep review — I've verified both blockers against `2a56e4fdb` and I agree they need to be resolved before this can merge. **On #1 (silent data loss with `auto_commit=false`):** Confirmed. The relaxation from `autoCommit && autoFlushed` to `autoFlushed` breaks the invariant that `pendingRows` must track every row in the open transaction scope. Once `rollbackIfNeeded()` runs, rows A1/A2 are gone from both the main sink and the error path, yet the deferred outcome queue can still report them as `WRITTEN` — that's the worst failure mode for a DLQ feature, since users will trust the error sink to be complete. The fix should keep transaction-scoped rows retained until commit (or explicitly re-report all rolled-back rows), and I'd like to see a regression test covering exactly the sequence you described: successful auto-flushed batch followed by a SQLState 22/23 failure within the same non-auto-commit transaction. **On #2 (query-level SQL failures classified as row errors):** Also confirmed. `SupportRowLevelErrorClassifier` must only classify errors that are attributable to a specific input row. Query/plan-level failures (bad SQL, unresolvable columns, engine state issues) affect every row and swallowing them under ROUTE mode turns a broken pipeline into a silently "successful" job that routes 100% of data to the error sink. The classifier in `ZetaSQLEngine`/`ZetaSQLFilter` needs to distinguish evaluation-time row errors from query-level errors and rethrow the latter unconditionally. **Concrete asks for the author before the next review round:** 1. Restore transaction-aware pending-row tracking in `JdbcSinkWriter` + the regression test above. 2. Tighten the SQL error classification so query-level exceptions always fail the job, with a test asserting ROUTE mode does not swallow them. 3. Please also address the MultiTable and checkpoint-metrics attribution items from this review — @nzw921rx, could you post the truncated remainder of those findings so they're fully on record? 4. A quick note in the PR description listing which findings were fixed per commit would help us re-verify efficiently. Given the silent-data-loss nature of #1, I'm marking this as request-changes. The overall design direction is still sound, and defaults remain backward compatible — these are fixable correctness gaps, not architectural problems. Thanks to both of you for the careful work here. <!-- streview-comment:90 --> -- 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]
