nzw921rx commented on PR #13054: URL: https://github.com/apache/seatunnel/pull/13054#issuecomment-6488911712
## Additional review and local validation Reviewed the complete PR diff and the JDBC transaction/checkpoint call chain. The close-path follow-up has been pushed as commit [`f86e91ba97c3`](https://github.com/nzw921rx/seatunnel/commit/f86e91ba97c323f895de8f0a4520f08cfce79e5b). - Final reviewed and tested head: `f86e91ba97c323f895de8f0a4520f08cfce79e5b` - Initial reviewed head: `dd975588a9ba2c53d79005b4c7f8fd560158aa6d` - Dev baseline used for negative controls: `886a3ac6e59aad5e46755761cf9fb8ac2a6d3df9` - Local environment: Linux, OpenJDK 17.0.20, Maven 3.9.11. - Compilation used the repository's Java 11 release configuration. This is separate from testing on a JDK 11 runtime. **Conclusion:** no remaining P0/P1 blocker was identified within the reviewed and locally tested scope. The shared transaction state is placed at the correct ownership boundary. The additional close-path gap found during validation has been fixed and covered by two committed regression tests. ### 1. Review scope All 17 changed files were reviewed, including: - `JdbcOutputFormat` - `JdbcTransactionStateManager` - `JdbcConnectionProvider` - `SimpleJdbcConnectionPoolProviderProxy` - `ConnectionPoolManager` - `JdbcSinkWriter` - The changed unit tests, MySQL multi-table integration test/configuration, and English/Chinese documentation. The review also followed the surrounding production paths: - `MultiTableSinkWriter` queue execution and checkpoint preparation. - `SinkFlowLifeCycle.processCheckpointBarrier()`. - Output-format rebuilding during schema changes. - The XA writer's separate transaction lifecycle. ### 2. Transaction-safety checks | Area | Verification | | --- | --- | | Shared connection ownership | Writers on the same queue index obtain the same transaction state manager. Different queue indexes have independent managers. | | Connection replacement | The manager outlives replacement of the cached connection. Committing a replacement cannot silently acknowledge batches pending on the previous connection. | | Flush retries | When earlier flushed work remains pending or has been lost, the writer rejects retrying only the current batch. | | Mixed exception chains | Connection loss or transaction rollback is recorded before either row-error handling or the non-retryable data-error path exits. | | Full rollback | The implementation distinguishes discarded rows reported by the current writer from previously reported good rows and other writers' pending work. | | Savepoint rollback | The shared flush sequence detects another writer's batches flushed after the savepoint. | | Successful shared commit | A successful commit clears pending work for all writers sharing that connection. | | Sticky transaction loss | Later flushes or rollback cleanup cannot make an already poisoned transaction eligible for commit. | | Format rebuilding | Schema changes prepare the current transaction before rebuilding the output format. Rebuilding cannot be used to bypass a rejected commit. | | Serialization | The format's transient transaction state is reconstructed on open; the serialization regression test is included in the executed suite. | | Unaffected paths | Auto-commit and commit-on-flush paths do not accumulate this pending-work state. XA requires `max_retries = 0` and does not use the non-XA commit-check protocol. | | Concurrency | Production multi-table operations sharing one queue connection are serialized under the corresponding queue runnable lock. | | Cost | The additional tracking occurs per successful flush, with state bounded by the writers sharing the queue. | This change preserves the existing connection-sharing model. It adds detection and commit rejection when that shared transaction can no longer account for previously flushed work. Configuration options, defaults, dependencies and checkpoint serialization are unchanged. The reduced retry behavior and cross-table rollback consequences are documented in English and Chinese. ### 3. Build and unchanged-head validation First, the JDBC module and its reactor dependencies were built from a clean state: - Nine reactor modules completed successfully. - Production and test sources compiled. - Test execution was skipped for this build step. The initial, unchanged PR head `dd975588a9ba` was then tested with the JDBC non-container suite: ```text Tests run: 1182 Failures: 0 Errors: 0 Skipped: 6 ``` That is **1176 passed and 6 skipped**, not 1182 executed passes. ### 4. Dev negative controls The baseline comparison used the actual dev implementations of these five affected production classes: - `JdbcOutputFormat` - `JdbcConnectionProvider` - `SimpleJdbcConnectionPoolProviderProxy` - `ConnectionPoolManager` - `JdbcSinkWriter` They were compiled separately into a baseline class directory. Separate JVM runs placed that directory before the PR production classes on the classpath and executed the same regression-test bytecode. The observed failures were assertion failures, not compilation or linkage failures. | Regression | Dev baseline | PR implementation | | --- | --- | --- | | `manualCommitWriterDoesNotDropEarlierBatchWhenLaterBatchReconnects` | Failed: the expected rejection was not thrown | Passed: unsafe retry was rejected | | `manualCommitWriterFailsCheckpointWhenConnectionIsReplacedBeforeCommit` | Failed: the expected rejection was not thrown | Passed: committing the replacement connection was rejected | These controls demonstrate that the tests detect the behavior this PR changes. A separate close-path diagnostic also failed on dev, as described below. That distinguishes the existing close behavior from a newly introduced checkpoint regression. ### 5. Additional local scenarios Nine additional scenarios were exercised by extending the existing `JdbcMultiTableReconnectTest`. | Scenario | Initial PR head | With the close fix | | --- | --- | --- | | Two consecutive row errors: a later full rollback discards previously reported good rows, so checkpoint preparation must fail | Passed | Passed | | Savepoints unavailable: all rows discarded by the writer's own rollback are reported, allowing a later valid commit | Passed | Passed | | Commit encounters a transaction rollback: another writer sharing the transaction must also reject checkpoint preparation | Passed | Passed | | Transaction loss on queue 0 must not incorrectly poison the independent state of queue 1 | Passed | Passed | | Connection replacement after a successful durable commit allows a newly opened writer to commit normally | Passed | Passed | | Real asynchronous multi-table queue: closing the shared pooled connection causes two successive checkpoint preparations to fail | Passed | Passed | | Real asynchronous multi-table queue: four healthy shared transactions commit successfully, with both tables visible after each checkpoint and pending state cleared | Passed | Passed | | Close handles a final-batch row error while retaining earlier good rows | Failed | Passed | | Close handles a mixed data/transaction error that poisons the transaction | Failed | Passed | The initial additional run therefore reported **9 tests, 7 passed, 2 failed**. The two failures were retained and investigated. The tests used real SQLite SQL execution. The pool and asynchronous coordinator scenarios used the production HikariCP, pooled provider and `MultiTableSinkWriter` wiring. Faults were injected at explicit batch/commit boundaries. The unsupported-savepoint case simulated the driver's capability response while retaining real SQLite SQL execution. These are deterministic connector-level fault tests, not real MySQL/Oracle/PostgreSQL failover tests. ### 6. Close-path finding and correction The initial close implementation handled a row error and then proceeded directly to resource closure: ```text close -> flush fails with a row-level error -> handleRowLevelBatchFailure -> outputFormat.close ``` It did not repeat the transaction completion/check performed by the corresponding prepareCommit path. #### Preserved-good-rows reproduction 1. Enable manual commit and row-error collection with `batch_size = 2`. 2. Write good rows 1 and 2. 3. Their automatic flush succeeds and establishes a savepoint, but the rows remain uncommitted. 4. Buffer row 3 and inject a constraint error into the final flush performed by close. 5. Row-error handling rolls back to the savepoint, preserving rows 1 and 2 in the open transaction. 6. Close returns normally. 7. SQLite connection closure rolls back the remaining transaction; the final query returns no rows. The same diagnostic failed on the dev baseline. A second diagnostic showed that close could also return normally after a mixed data/transaction error had poisoned the transaction. #### Checkpoint-path classification This was treated as an existing close-path gap, not as proof that the PR introduced a new normal Zeta checkpoint data-loss path. The inspected Zeta checkpoint path invokes `writer.prepareCommit()` before continuing with snapshot/ACK processing. The normal Spark commit path also prepares before closing. Directly reproducing the close gap does not establish that those normal checkpoint paths bypass preparation. #### Applied fix Commit `f86e91ba97c3` adds seven production lines: - After handling the close-phase row error, call `commitIfNeeded()`. - Commit preserved good rows when the transaction remains valid. - Reject an already poisoned transaction. - If commit checking or submission fails, attempt rollback and propagate the failure before resource closure. Two regression tests were committed: - `closeRowErrorCommitsEarlierGoodRows` - `closeRowErrorRejectsPoisonedTransaction` The minimal patch was also applied to a separate clean worktree and both tests passed. Before pushing, the tests were rerun under their final committed names and formatting checks passed. ### 7. Extended-suite and final-head results | Tested version | Total | Passed | Failures | Errors | Skipped | | --- | ---: | ---: | ---: | ---: | ---: | | Initial unchanged head `dd975588a9ba` | 1182 | 1176 | 0 | 0 | 6 | | Local extended tree: close fix plus all nine additional scenarios | 1191 | 1185 | 0 | 0 | 6 | | Final pushed head `f86e91ba97c3`, verified in a clean worktree | 1184 | 1178 | 0 | 0 | 6 | The distinction between these counts is intentional: - The final PR contains the two close regression tests. - The other seven additional scenarios were local review coverage. - **1191 is the extended local-tree result; 1184 is the final pushed-head result.** - These overlapping runs must not be summed as distinct test coverage. Additional checks: - Nine-module reactor verification with test execution skipped: passed. - Scoped Spotless apply/check: passed. - `git diff --check`: passed. - The remote follow-up commit's tree matched the validated local tree. - The final pushed-head suite was rerun after checking out `f86e91ba97c3` in a clean worktree. ### 8. Reproduction commands The following are the equivalent project commands; Maven 3.9.11 was used for the local runs. Build the JDBC reactor dependencies: ```bash ./mvnw -B -ntp \ -pl seatunnel-connectors-v2/connector-jdbc -am \ -DskipTests -DskipIT=true \ clean install ``` Run the JDBC non-container suite: ```bash ./mvnw -B -ntp \ -pl seatunnel-connectors-v2/connector-jdbc \ '-Dtest=**/*Test.java,!**/internal/dialect/kingbase/container/*,!**/internal/dialect/oracle/container/*' \ -DskipIT=true \ test spotless:check ``` Run the two committed close regressions: ```bash ./mvnw -B -ntp \ -pl seatunnel-connectors-v2/connector-jdbc \ '-Dtest=JdbcMultiTableReconnectTest#closeRowError*' \ -DskipIT=true \ test spotless:check ``` Verify the scoped reactor: ```bash ./mvnw -B -ntp \ -pl seatunnel-connectors-v2/connector-jdbc -am \ -DskipTests -DskipIT=true \ spotless:check verify ``` The local test JVM additionally used `-XX:ActiveProcessorCount=2`, `-Xmx2g` and an explicitly loaded Byte Buddy 1.12.19 agent to avoid runtime self-attachment limitations. ### 9. Coverage boundaries and CI status - Docker was unavailable locally. The four Oracle/Kingbase container test classes were excluded; six existing skipped tests remain reflected in the totals. - The MySQL multi-table integration test and its configuration were reviewed but not executed locally. - Real external-database connection termination, deadlock/failover and complete Zeta recovery/replay were not executed locally. - SQLite/HikariCP/coordinator testing establishes the observed connector commit rejection and healthy commit behavior; it is not a substitute for those external-database E2E runs. - JDK 11 runtime, Windows and the full repository test suite were not run locally. - Java 11 release compilation does not establish JDK 11 runtime coverage. - The final-head [public Build](https://github.com/apache/seatunnel/runs/123279670826) was queued when checked. After the close follow-up, no remaining code blocker was identified within this review's scope. The final-head public CI result and required maintainer review remain outstanding. -- 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]
