LuckysHorizon commented on PR #13136: URL: https://github.com/apache/seatunnel/pull/13136#issuecomment-6683298625
> Thanks for the quick and careful follow-up, @LuckysHorizon, and for explicitly confirming that the reworked distributed test fails when history is moved ahead of the distributed lookup (`expected: <true> but was: <false>`) - that is exactly the regression power the previous round asked for. I re-checked [6f745fc](https://github.com/apache/seatunnel/commit/6f745fc2497bca627d64fa2478af1c03db7271e0) against your comment: the blocking test issue is fixed and the helper comments are in. The only thing left is CI: no Build has ever run for this branch, and it is not only waiting on workflow approval - your fork has no Actions runs at all, so please enable GitHub Actions on the fork at https://github.com/LuckysHorizon/seatunnel/actions and then push an empty commit or re-run so the Build check attaches here. Separately, the apache-side `PR signal` run for this head (https://github.com/apache/seatunnel/actions/runs/42200431841) is in `action_required` and needs a maintainer to approve i t, as you noted. > > Reviewed head `6f745fc2497bca627d64fa2478af1c03db7271e0`; incremental diff since my last review (`2b541892`) touches only `CoordinatorServiceTest.java` and comment lines in `RealtimeMetricsRestIT.java`. Production code is unchanged since the last round. > > # What Problem Does This PR Solve? > **User pain point:** A same-ID savepoint restore leaves the previous attempt in job history, and `CoordinatorService.getJobInfo()` read history first, so realtime observability kept the old `enabled` / `bucket_ms` / `retention_minutes` values (issue #13130). > > **Fix approach:** Resolve metadata as running JobMaster -> pending JobMaster -> distributed `JobInfo` -> history. > > **One-sentence summary:** The current attempt's configuration wins over retained history of a previous attempt with the same job ID. > > # 1. Code Change Review > ## 1.1 Core Logic Analysis > `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1910-1936` (unchanged since last round): > > ```java > JobMaster runningJobMaster = runningJobMasterMap.get(jobId); > if (runningJobMaster != null) { return runningJobMaster.getJobDAGInfo(); } > PendingJobInfo pendingJobInfo = pendingJobQueue.getById(jobId); > if (pendingJobInfo != null) { return pendingJobInfo.getJobMaster().getJobDAGInfo(); } > JobInfo runningJobInfo = runningJobInfoIMap.get(jobId); > if (runningJobInfo != null) { return restoreJobDAGInfo(runningJobInfo); } > JobDAGInfo jobInfo = jobHistoryService.getJobDAGInfo(jobId); > if (jobInfo != null) { return jobInfo; } > throw new JobNotFoundException(...); > ``` > > ``` > Savepoint stop -> history retains attempt N > Same-ID submit -> runningJobInfoIMap(attempt N+1) -> pending -> running > RealtimeMetricsService.collectOnce -> resolveObservabilityConfig > -> CoordinatorService.getJobInfo > running / pending JobMaster DAG (current attempt) > else restoreJobDAGInfo(distributed) (current attempt, init/takeover window) > else history (no current attempt) > -> ObservabilityConfig -> stores -> jobs/edges/vertices REST > ``` > > Previous blockers re-checked against this head: > > Previous item Status > (Round 2, blocking) Distributed precedence test could not distinguish history-first from distributed-first **Fixed.** `CoordinatorServiceTest.java:1511-1590` now waits until the submitted job is FINISHED and its `runningJobInfoIMap` entry is gone, seeds a real serialized `JobImmutableInformation` with `enabled=true, bucket_ms=2000`, mocks history with `enabled=false, bucket_ms=1000`, and asserts the distributed values (`:1584-1585`). `restoreJobDAGInfo` takes env options from the deserialized `LogicalDag` job config (`DAGUtils.getJobDAGInfo`), so the seeded values are what the lookup returns; reversing the order makes the assertion fail. > (Round 2, non-blocking) Comment the restore fixture and readiness helpers **Fixed.** `RealtimeMetricsRestIT.java:383-385` and `:437-438`. > (Round 1) Restore coverage and precedence comment Fixed in the previous round, still present. > ## 1.2 Compatibility Impact > **Fully compatible.** No public signature, option key, default, REST shape, or serialized format changes. The only behavior change is the intended one: when a current attempt and older history coexist for one job ID, the current attempt is returned. > > ## 1.3 Performance / Side-Effect Analysis > No new threads, locks, buffers or polling. Running/pending lookups now skip the history read. The new test uses `state-cleanup-delay-ms: 0` from the test `seatunnel.yaml`, so the 60s wait for the distributed entry to be removed is condition-based and not timing-sensitive; the re-inserted entry has a new initialization timestamp, so any late cleanup of the finished attempt cannot remove it (`CoordinatorService.java:906-909`). > > ## 1.4 Error Handling and Logging > `JobNotFoundException` remains the absent-metadata result. No new logging. > > No new code issues found in this round. > > # 2. Code Quality Assessment > ## 2.1 Coding Standards > Fine. Optional nit only: `CoordinatorServiceTest.java:1553` sets `runningJobInfoIMap` via reflection to the same `IMAP_RUNNING_JOB_INFO` instance the service already holds; it is harmless, so feel free to leave it. > > ## 2.2 Test Coverage and Test Stability > **Stable.** `CoordinatorServiceTest.java:1472-1590` and the other new precedence cases use distinct current/history values and exact assertions, with shutdown in `finally`. `RealtimeMetricsRestIT.java:247-373` is honestly described as restore-flow smoke coverage, and the unit tests carry the regression proof. The e2e change in this commit is comment-only, but its compile has not been confirmed by CI yet. > > ## 2.3 Documentation Updates > EN and ZH notes are both present and scoped to restored-attempt settings. > > # 3. Architectural Soundness > The fix is at the right owner (`CoordinatorService` metadata precedence) and is the smallest correct change. #13133 is still open and overlaps on the running/pending branches; this PR additionally covers the distributed-only window. Maintainers should land only one of the two. > > # 4. Issue Summary > No. Issue Location Severity > - No open code issues - - > CI No Build has run on this head (fork Actions not enabled); `PR signal` awaits maintainer approval https://github.com/LuckysHorizon/seatunnel/actions Blocking (CI only) > # 5. Merge Recommendation > ### Conclusion: Ready to merge after fixes > 1. **Blockers:** CI only - enable Actions on the fork so the Build check runs on this head, and a maintainer needs to approve the `PR signal` run. No non-CI blockers remain. > 2. **Non-blocking:** the optional reflection nit above; coordinate with #13133 so only one lookup fix lands. > > The branch is 5 commits behind `dev` with no conflicts; that alone does not need a sync. I am leaving this as COMMENT because CI has not run yet; I will be happy to approve once the Build is green on this head. Nice work on this one. Update on local verification of head 718f00b8d. My fork's Actions run could not start because of a GitHub billing lock on my account, so I ran the checks locally: - E2E module compiles: `./mvnw -pl seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base -am -DskipTests install` finished with BUILD SUCCESS (I excluded the seatunnel-engine-ui module, whose `npm install` fails in my environment). A forced `test-compile` on the e2e module then recompiled all 71 test sources, including RealtimeMetricsRestIT, with BUILD SUCCESS. So the comment-only edits compile. - CoordinatorServiceTest: 44 run, 0 failures, 2 skipped. - spotless:check and git diff --check pass. Note: I did not re-run RealtimeMetricsRestIT after the comments-only commit; it passed 2/2 locally before that. @DanielLeens, this should close the compile gap you mentioned, apart from the apache-side Build. Could a maintainer please approve the pending apache-side workflow run when convenient? Thanks! -- 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]
