xinnyuli opened a new pull request, #12625:
URL: https://github.com/apache/seatunnel/pull/12625
[Fix][Connector-V2] Fail instead of committing when JDBC sink loses
uncommitted batches
With checkpointing and a manual-commit connection, batch-size flushes stay
in one open transaction until the checkpoint commit. A flush retry after a
lost connection or a transaction rollback re-sent only the current batch,
and a pooled connection replaced before commit was committed empty. In both
cases the checkpoint succeeded while earlier flushed rows were gone.
Track the connection that holds flushed but uncommitted batches. Do not
retry a flush while it is set, or when the database rolled back the whole
transaction (SQLState class 40). Fail the commit if the connection being
committed is not that connection.
Closes #12624
```
---
### Purpose of this pull request
Fixes #12624
With checkpointing enabled, `JdbcSinkWriter` commits only in
`prepareCommit`, timer flush and close. On a manual-commit connection
(`auto_commit = false`, or Oracle), every batch-size flush between two commits
sits in one open transaction. On dev that transaction can be lost without the
sink noticing, and the next checkpoint still succeeds:
1. **Retry path (`max_retries > 0`).** A later flush fails with a lost
connection (`08xxx`) or a whole-transaction rollback (`40001`).
`JdbcOutputFormat.flush` reconnects if needed and retries, but the retry only
re-sends the current batch. The earlier batches were rolled back with the old
transaction.
2. **Pool path (default config).** `ConnectionPoolManager.getConnection()`
silently replaces a dead cached connection. `commitIfNeeded()` then commits the
new, empty connection and the checkpoint succeeds. Everything flushed on the
dead connection since the last checkpoint is gone.
Changes:
- `JdbcOutputFormat` records the connection the executor was prepared on.
After a successful flush on a manual-commit connection with `commitOnFlush ==
false`, it remembers that connection as holding uncommitted batches.
- In the flush retry loop, a failed flush is not retried while such batches
exist, or when the exception chain has SQLState class `40`. The class-40 check
matters even without batches of this writer's own, because writers in the same
multi-table queue share one pooled connection and transaction. The flush fails
with `FLUSH_DATA_FAILED`, so the job recovers from the last checkpoint.
- `JdbcSinkWriter.commitIfNeeded()` calls
`outputFormat.checkUncommittedBatchesOn(connection)` before committing. If the
connection about to be committed is not the one that holds the batches, it
fails with `TRANSACTION_OPERATION_FAILED` instead of committing the replacement.
- After a commit or a full rollback, the writer calls
`markTransactionEnded(connection)`. It clears the record only when the
connection that was ended is the recorded one, so rolling back a replacement
connection does not hide the lost batches. A rollback to a savepoint keeps the
record, because the batches up to the savepoint are still uncommitted.
### Does this PR introduce _any_ user-facing change?
Yes, only for jobs that use checkpointing **and** a manual-commit connection
(`auto_commit = false`, or Oracle):
- **Before:** a flush failure after an earlier uncommitted flush could be
retried, and a replaced connection could be committed. The checkpoint succeeded
with rows missing.
- **After:** the job fails and recovers from the last checkpoint. No rows
are lost.
As a side effect, `max_retries` now has no effect after the first flush in
each checkpoint interval on these connections. It also no longer retries
class-40 errors there. That includes errors that only roll back the failing
statement on some databases, for example a lock wait timeout. Such jobs now
restart from the checkpoint instead of retrying in place. This is the safe
choice, because the sink cannot tell from the error alone what the database
kept.
Auto-commit connections, jobs without checkpointing (`commitOnFlush`), and
the XA / exactly-once writer behave as before.
Docs: added a paragraph under `max_retries` in
`docs/en/connectors/sink/Jdbc.md` and `docs/zh/connectors/sink/Jdbc.md`.
### How was this patch tested?
Unit tests:
- `JdbcMultiTableReconnectTest` (real SQLite, through `JdbcSinkWriter`):
- `manualCommitWriterDoesNotDropEarlierBatchWhenLaterBatchReconnects`:
covers path 1. On dev, the retry succeeds and rows 3–4 are committed without
rows 1–2. With this PR the write fails and nothing partial is committed.
- `manualCommitWriterFailsCheckpointWhenConnectionIsReplacedBeforeCommit`:
covers path 2. The test provider replaces a closed connection on
`getConnection()`, like `ConnectionPoolManager`. On dev `prepareCommit`
succeeds. With this PR it fails.
- `JdbcOutputFormatReconnectTest` (Mockito):
- No retry or reconnect while an earlier batch is uncommitted, for both
`08006` and `40001`.
- No retry on a class-40 error even without an earlier batch (shared
connection).
- A class-40 error on an auto-commit connection is still retried.
- Retry and reconnect still work after the writer ended the transaction,
and on auto-commit connections.
- The commit check fails on a replaced connection, and only ending the
original connection's transaction clears it.
Both `JdbcMultiTableReconnectTest` tests fail on dev with only the test
change applied, and pass with the fix. The whole `connector-jdbc` test suite
and `spotless:check` pass:
https://github.com/xinnyuli/seatunnel-12382-repro/actions/runs/37205833000
I didn't add an e2e test. Path 1 needs a connection drop at an exact point
between two flushes, and path 2 needs one between a flush and the checkpoint.
Both can be triggered deterministically through the writer with SQLite, which
the unit tests do. A timing-based e2e test (for example `pg_terminate_backend`
in `connector-jdbc-e2e`) would be flaky. Happy to add one if reviewers want it.
Known limitations, not changed here:
- In row-error mode, `reOpenOutputFormat` after a savepoint rollback creates
a new output format, so the record of earlier uncommitted batches is not
carried over.
- If `getAutoCommit()` throws, the sink assumes manual commit. This is the
fail-safe direction: at worst an auto-commit job fails and recovers instead of
retrying.
### Check list
* [x] No new Jar binary package.
* [x] Documentation updated (en + zh `Jdbc.md`).
* [ ] `incompatible-changes.md`
* [x] Not a new connector.
--
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]
