SEZ9 commented on issue #12624: URL: https://github.com/apache/seatunnel/issues/12624#issuecomment-6007915981
Thanks @xinnyuli, that matches the direction we discussed, and it's good to hear both `JdbcMultiTableReconnectTest` cases plus `JdbcOutputFormatReconnectTest` are already in the PR. I'll review it there; a few follow-ups on the points you raised: 1. **Flush retry**: failing the task whenever earlier batches are still uncommitted, or on SQLState class 40, is the behaviour we want. The carve-out for a connection error (08xxx) on the first batch after a commit sounds safe to me, since nothing accepted on the old connection is lost and re-sending that batch is complete. Two things I'd like to confirm in the PR: (a) the "earlier uncommitted batches" marker is cleared only after a *successful* commit (not on rollback or reconnect), and (b) in the multi-table case the marker is tracked per connection rather than per table, so a flush for table B after a flushed-but-uncommitted batch for table A on the same connection is still treated as unsafe to retry. 2. **Replaced connection**: having `commitIfNeeded()` call `checkUncommittedBatchesOn(connection)` first and throw if the connection that held the flushed batches was swapped is exactly the shape I had in mind. Could you briefly describe in the PR how the check identifies "the connection that held the batches" (e.g. identity of the connection object vs. something else), so reviewers can reason about the pooled case where a replacement might otherwise look identical? 3. **Oracle**: reading the actual `getAutoCommit()` state rather than the `auto_commit` option is the right choice, and since `resolveSinkAutoCommit()` always returns `false` for Oracle, that covers the default Oracle configuration. Your comment got cut off at "I'll add this" — could you finish that thought? A note in the PR description stating that Oracle goes through the same manual-commit path is sufficient for me; an Oracle-specific test would be nice but I don't consider it blocking given the SQLite manual-commit tests exercise the same code. Once those are covered I'll do a full pass on the PR. <!-- streview-comment:1539 --> -- 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]
