LuckysHorizon commented on PR #13136:
URL: https://github.com/apache/seatunnel/pull/13136#issuecomment-6660150496
> Thanks for contributing to Apache SeaTunnel! The lookup correction makes
sense. There is one coverage gap to close before merge, and #13133 already
contains an overlapping implementation worth coordinating with.
>
> Reviewed head `c2a6c78964e619ae239c7cce9c0ceebd7e188112` against base
`a3743d7f52e9200a7733857ca813a0836bf66bba`. Report time:
2026-11-05T01:05:00.165514+00:00.
>
> # What Problem Does This PR Solve?
> **User pain point:** A savepoint restore reuses a job ID, but retained
metadata can describe the earlier attempt. Realtime metrics can therefore
remain disabled after the restored job enables them.
>
> **Fix approach:** Prefer the active or pending JobMaster when resolving
job metadata.
>
> **One-sentence summary:** The metrics collector reads the restored attempt
configuration instead of the prior attempt history.
>
> **Example:** Stop job 42 with observability disabled, then restore job 42
with it enabled: the previous lookup still returned false; the new
active-attempt lookup returns true.
>
> # 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`
moves history after running, pending and distributed metadata. This also
changes the distributed-only boundary, beyond the running/pending precedence
described in the PR.
>
> **Before / after:**
>
> ```
> Before: history -> running JobMaster -> pending JobMaster -> distributed
JobInfo
> After: running JobMaster -> pending JobMaster -> distributed JobInfo ->
history
> ```
>
> **Key findings:**
>
> * Normal realtime collection reaches the modified lookup.
> * Active same-ID restores now supersede old history.
> * Distributed-only metadata also wins over history, which differs from
#13133.
> * Added tests prove running precedence, history-only fallback and
unchanged options; they do not exercise a real restore.
>
> **Complete runtime path:**
>
> ```
> Same-ID restore submission -> distributed JobInfo -> JobMaster.init ->
pending queue -> running map
> RealtimeMetricsService.collectOnce (:220) -> resolveObservabilityConfig
(:300)
> -> CoordinatorService.getJobInfo (:1910) -> current DAG env options
> -> enabled/disabled store handling -> metrics REST endpoints
> No current master -> distributed JobInfo deserialization (:1935) ->
history only if absent
> ```
>
> **Correctness and boundaries:** The active and pending branches return the
current DAG directly. The distributed-only branch deserializes the current
logical DAG and now precedes history too; that can be reached during submission
initialization or master takeover. It deserves an explicit conflict-of-sources
test. No state format or metrics-store reset is implemented here, so the title
should not imply broader state reinitialization.
>
> ## 1.2 Compatibility Impact
> **Fully compatible.** No public signatures, options, defaults, protocol
fields or checkpoint serialization change. The intended historical-behavior
correction is the metadata chosen when old history and a current same-ID
attempt coexist.
>
> ## 1.3 Performance / Side-Effect Analysis
> Running/pending lookups now avoid the history lookup. No new buffers,
threads, locks or network polling are introduced. The existing periodic
collector still refreshes metadata, removes disabled stores and rebuilds stores
when bucket/retention settings change. Resource release, retries and checkpoint
idempotency are unchanged; this does not promise to reset same-configuration
metric windows on every restore.
>
> ## 1.4 Error Handling and Logging
> Missing jobs still raise JobNotFoundException. Existing collector error
handling remains unchanged; no new credential logging or success masking was
added.
>
> ### Issue 1: Cover the real savepoint restore and all changed precedence
branches
> * **Location:**
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1462`
> * **Problem description:** The added tests inject a running mock and call
getJobInfo directly. They do not stop/restore a real job or observe the metrics
collector, and do not cover pending or distributed-only current metadata
coexisting with conflicting history. The existing RealtimeMetricsRestIT is
unchanged.
> * **Potential risk:** The unit tests can pass while the restore lifecycle
or realtime endpoint continues exposing old settings. The repository requires
extending the existing integration test for this changed engine path.
> * **Best improvement:** Extend RealtimeMetricsRestIT with a
disabled-to-enabled same-ID savepoint restore and assert changed configuration
plus live metrics. Add focused pending/history and distributed/history
precedence cases. Coordinate with #13133 to reuse the overlapping regression
rather than duplicating maintenance.
> * **Severity:** Medium (blocking)
> * **Raised by another reviewer:** No
>
> ### Issue 2: Explain the new lookup precedence at the core method
> * **Location:**
`seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1910`
> * **Problem description:** The modified public metadata lookup has no
comment documenting why current and distributed metadata precede retained
history.
> * **Potential risk:** The attempt-ownership invariant is easy to reverse
during future cleanup; no additional runtime failure is claimed.
> * **Best improvement:** Document the current-attempt precedence and the
intended distributed-only fallback boundary.
> * **Severity:** Medium (non-blocking)
> * **Raised by another reviewer:** No
>
> # 2. Code Quality Assessment
> ## 2.1 Coding Standards
> The patch is focused and preserves imports. The core precedence contract
should be documented (Issue 2).
>
> ## 2.2 Test Coverage and Test Stability
> **Stable.**
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1462`,
:1503 and :1526 use deterministic mock state, exact identity/configuration
assertions and finally shutdown. These are useful unit regressions, but not
integration execution evidence; Issue 1 covers the missing layer.
>
> **Local verification:** `git fetch` obtained the reviewed head; an
independent `seatunnel-review-13136` worktree was checked out. `git diff
a3743d7f52e9200a7733857ca813a0836bf66bba...c2a6c78964e619ae239c7cce9c0ceebd7e188112`
covered all 2 changed files, reconciled with GitHub's complete files list.
`git diff --check` returned 0. Source searches and caller inspection support
the paths above. No local build, unit test, integration test, or PR runtime
execution was performed, as requested. CI is separate evidence.
>
> ## 2.3 Documentation Updates
> No new setting is added. An EN/ZH note describing restored-attempt
precedence would make this user-visible correction clearer, as in #13133; no
format migration entry is needed.
>
> # 3. Architectural Soundness
> ## 3.1 Elegance of the Solution
> **Precise fix.** CoordinatorService owns metadata precedence, so
correcting the lookup here is preferable to teaching the metrics collector to
bypass it. The smallest fix and best responsibility boundary coincide.
Resetting metrics stores or changing savepoint bytes would enlarge scope
without addressing the stale lookup.
>
> ## 3.2 Maintainability
> A linear precedence chain is easy to inspect; preserve explicit
distinction between a current attempt and terminal history.
>
> ## 3.3 Extensibility
> Other consumers of this metadata resolution also obtain the current
attempt without a connector-specific exception.
>
> ## 3.4 Historical-Version Compatibility
> Existing savepoints and JobDAGInfo formats are unchanged. Historical-only
lookups remain available when no current attempt is present; no migration is
needed.
>
> # 4. Issue Summary
> Number Issue Location Severity
> 1 Cover the real savepoint restore and all changed precedence branches
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1462`
Medium
> 2 Explain the new lookup precedence at the core method
`seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1910`
Medium
> Full-scan check: complete diff, affected callers, failure/lifecycle
branches, compatibility, test behavior/stability, documentation and current
merge gates examined. Existing reviews and inline/discussion comments were
read; no other reviewer had a technical finding or approval/change-request
intent to reconcile in the collected snapshot.
>
> # 5. Merge Recommendation
> ### Conclusion: Ready to merge after fixes
> 1. **Blockers — must be fixed:** Issue 1: Cover the real savepoint restore
and all changed precedence branches No Build check was reported in the
collected head snapshot; a current-head CI result is also required. Missing
checks alone do not prove workflows are disabled.
> 2. **Recommended fixes — non-blocking:** Issue 2: Explain the new lookup
precedence at the core method
>
> **Overall assessment and alternatives:** The source direction is sound.
Prefer one coordinated implementation with explicit fallback semantics and
restore coverage. Do not merge both overlapping patches independently.
>
> Upstream snapshot: `dev`, ahead 1, behind 0, compare `ahead`, merge state
`BLOCKED`. Behind/diverged alone is metadata, not a request to synchronize.
Daniel has READ permission; this COMMENT is a technical review, not a
write-capable maintainer approval.
Thanks for the detailed review, @DanielLeens. I pushed 2b541892e to address
it.
Issue 1 (coverage):
- Added unit tests in CoordinatorServiceTest for pending-vs-history,
distributed-only (runningJobInfoIMap)-vs-history, the same attempt present in
both distributed metadata and history, history-only fallback, and the not-found
case. The precedence tests fail against the previous ordering and pass with
this change.
- Added a same-ID savepoint restore test to RealtimeMetricsRestIT (disabled
-> savepoint -> restore with enabled=true and a different bucket_ms), asserting
the jobs/edges/vertices endpoints and live vertex points. To be upfront: in
this harness it also passes under the old ordering, so I'm treating it as
end-to-end smoke coverage of the restore flow, and the unit tests carry the
regression proof.
Issue 2 (comment): added a comment at getJobInfo() explaining that history
may belong to a previous attempt, that the running/pending JobMaster and
distributed JobInfo take precedence, and that history is the fallback when no
current attempt exists.
Docs: added a short EN/ZH note on restored-attempt behavior.
On the distributed-metadata ordering: this is intentional. A restored job
can exist in the distributed JobInfo map before its JobMaster is registered
(submission init or master takeover) while history still holds the previous
attempt, so history-after-distributed avoids returning the old config in that
window. The delayed cleanup of an old attempt's runningJobInfoIMap entry is
guarded by the initialization timestamp, so it cannot remove a new attempt's
entry. One trade-off: for a finished job during the cleanup window, the
reconstructed DAG has the same edges, vertices and env options as history, but
the current master address and an empty historyExecutionPlan. The comment and a
test cover that case.
I also retitled the PR, since it only corrects the metadata lookup and does
not reset metrics stores.
Regarding #13133: it overlaps on the running and pending branches. I'm happy
to coordinate so only one lands, whichever the maintainers prefer, but I think
the distributed-only case should be covered either way.
--
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]
