DanielLeens commented on PR #12043: URL: https://github.com/apache/seatunnel/pull/12043#issuecomment-6538138267
Synced the branch with `dev` and fixed the failing new test from `a81d807f13a`. New head: `248d243d5d890e130872e8486bdb846b6e6bbbd2`. **Merge of `dev` (`3ac2d1cec6a`)** GitHub reported a conflict; it came from `monitor-history-size` landing on `dev` next to this PR's `restore-progress-*` options. Five files conflicted, all resolved by keeping both sides (dev's option and this PR's options, no logic changed): - `docs/en/engines/zeta/hybrid-cluster-deployment.md`, `docs/zh/engines/zeta/hybrid-cluster-deployment.md` (option description blocks and the example yaml) - `seatunnel-engine-common/.../YamlSeaTunnelDomConfigProcessor.java` (adjacent `else if` branches) - `seatunnel-engine-common/.../YamlSeaTunnelConfigParserTest.java` and `src/test/resources/seatunnel.yaml` (adjacent assertions / yaml keys) `CheckpointCoordinator.java`, `SubPlan.java`, `JobMaster.java` and the server tests merged without conflict. `git diff origin/dev --name-status` after the merge shows the same 18 PR-owned files and no deletions. **Root cause of `CheckpointRestoreProgressConcurrencyTest.restoreDuringCheckpointStorageStillPublishesCompletion` failing on every unit-test lane** The failure (`AssertionFailedError: expected true but was false` inside the `storeCheckPoint` stub, surfaced through `completion.get`) is a deadlock in the test's own choreography, not a production defect: - `CheckpointCoordinator.completePendingCheckpoint` is `synchronized`, so the completion thread holds the coordinator monitor for the whole blocked `storeCheckPoint` call (`CheckpointCoordinator.java:1452`, `:1472`). - The test then called `coordinator.restoreCoordinator(false)` from the test thread. `restoreCoordinator` swaps the future in `prepareRestoreProgressTracking()` (`:735`, `:1623-1630`) and then calls `updateStatus(RUNNING)` (`:737`), which is `private synchronized` (`:1868`) and blocks on the monitor the completion thread holds. This serialization is pre-existing `dev` behaviour (dev's `restoreCoordinator` also calls the synchronized `updateStatus` right after replacing the future). - `releaseStorage.countDown()` sat in the `finally` after `restoreCoordinator` returned, so nobody could release storage; after 5 s the stub's `releaseStorage.await(5, SECONDS)` returned false, the assertion error escaped `storeCheckPoint` via `sneakyThrow`, the monitor was released, the restore went through, and `completion.get` rethrew the error. That matches the CI stack (`:365 <- lambda :339 <- :357`). The production fix from `a81d807f13a` is right for the blocker: the future is replaced before the monitor is taken, and the narrowed guard (`completionFuture.isDone() || shutdown`, `:1509`) no longer treats that replacement as a reason to discard the stored checkpoint. The test just could not express the race from a single thread. **Fix** (`CheckpointRestoreProgressConcurrencyTest.java`, only this test method; no assertion removed) - The restore now runs on the second executor thread. The test waits until `coordinatorFuture() != completionFuture`, which happens before the restore blocks on the monitor, so it still proves the restore raced the in-flight storage call, then releases storage and joins both futures. - Assertions after both threads finish: `sendOperationToMemberNode` called once (tasks notified), `coordinator.isCompleted()` (the savepoint became `latestCompletedCheckpoint`), the new future resolves to `SUSPEND` (the completion landed in the lifecycle the restore installed instead of being dropped), and the persisted status is `RUNNING`. The last one replaces the earlier `SUSPEND` expectation: the restore only reaches `updateStatus(RUNNING)` after the completion thread released the monitor (which already wrote `SUSPEND`), so under the real lock order the restore's write is last. The old expectation encoded the ordering that the coordinator monitor forbids. A Javadoc on the test explains the thread layout. **Open for the reviewer** The `|| shutdown` leg of the guard at `:1509` is PR-authored; `dev` has no such check and always publishes after storage. If a restore follows a `cleanPendingCheckpoint(...)` that set `shutdown = true` while a completion is still inside storage, that completion is skipped (with the new INFO log). I did not change it here; flagging so it can be judged together with Issue 1. **Validation** Module-scoped `spotless:apply` and `spotless:check` on `seatunnel-engine-common` and `seatunnel-engine-server` passed. Compile and tests are left to CI per the validation policy; fork Build run for this head: https://github.com/DanielLeens/seatunnel/actions/runs/41162454407 -- 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]
