nzw921rx commented on PR #12042: URL: https://github.com/apache/seatunnel/pull/12042#issuecomment-6474479026
# PR #12042 Review Reviewed head `7cfd9a7c23` and validated a local merge of `dev` at `f7a575432f`. **No blocking production-code issue found within the reviewed and tested paths.** The two previously failing `CheckpointCoordinatorTest` fixtures are fixed, with their original assertions retained. ## Code review - Restore eligibility remains narrow: every coordinator must be `RUNNING`. Finished physical groups qualify only when they contain sources and all sources are unbounded. - Failed, unknown, bounded, empty, non-source and mixed-boundedness groups remain excluded. - `closedIdleTask` is reconstructed before triggering another checkpoint, excluding exited readers and their fused downstream tasks from required acknowledgements. - Missing source metadata is rejected. - Each physical restore decision reads execution state once. The coordinator's subsequent IMap read and the state-only classification assumptions are documented. ### `pipelineTaskStatus` / `CLOSED` I checked the production call sites. Tasks report `WAITING_RESTORE` and `READY_START`; I found no normal idle-close path reporting `CLOSED`. The concern that missing reconstructed `CLOSED` entries changes the checkpoint shape is therefore not supported by the current implementation. An additional integration test verifies that: 1. Checkpoint state and task-statistics key sets remain consistent across master failover. 2. A new job can restore through the public checkpoint-restore API using a checkpoint completed after failover. 3. The restored job completes at least three distinct persisted checkpoints. 4. The exact 20-row contents remain unchanged, and cancellation succeeds. ## Local validation | Validation | Result | | --- | --- | | Clean source build: Engine server and its 40-module reactor | Passed | | Engine client clean source build | Passed | | Unmodified tests across 22 classes | 85 passed; 2 pre-existing disabled | | Three relevant classes with additional local tests | 32 passed | | Original `CheckpointCoordinatorFailoverIT` | 5 passed | | Additional Engine integration scenarios | 3 passed | | Spotless and diff whitespace checks | Passed | The additional local tests cover: - Repeated restoration without accumulating stale closed-task entries. - Three finished fused groups containing nine closed tasks, with eight live tasks sending concurrent duplicate acknowledgements. - Every execution state and `null` in the finished-reader predicate. - All readers idle-closed, followed by two consecutive master switches. - Abrupt master termination through `LifecycleService.terminate()`. - Restoration from a checkpoint completed after master failover. The consecutive-switch test preserves worker identity and exact row contents. It observes three distinct persisted completed checkpoints after each switch: `[5, 6, 7]` and `[13, 14, 15]`. The two pre-existing disabled tests were not counted as passed: - `CheckpointSerializeTest.testPipelineStateDeserialize` - `SavePointTest.testSavePointOnServerRestart` ## Negative control The same unmodified idle-reader failover test was run against separately cleaned and compiled production code: | Production code | Result | | --- | --- | | `dev` at `f7a575432f` | Timed out after two minutes waiting for a newer completed checkpoint after failover | | PR with the local `dev` merge | Passed in approximately 21 seconds | Production class hashes were recorded. The PR compilation remained isolated from the dev control. ## Validation scope Validation used Linux, JDK 17, embedded Hazelcast master/worker nodes and real FakeSource → LocalFile jobs. This does not establish coverage for external CDC databases, multi-process deployments, JDK 11, Windows, cross-version binary restoration, or simultaneous master/worker loss. The inspected diff does not change dependencies, configuration defaults, SPI registrations, serialized fields or checkpoint/savepoint formats. ## Recommendation **I am comfortable approving the code based on this review.** The `dev` merge was validated locally and has not been pushed to this PR. Please synchronize the PR branch and ensure the final head has a complete successful CI result before merging. -- 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]
