LuckysHorizon commented on PR #13136:
URL: https://github.com/apache/seatunnel/pull/13136#issuecomment-6682451101
> Thanks for contributing to Apache SeaTunnel! Thanks for adding the restore
smoke test, explaining the distributed window and acknowledging the overlap
with #13133. The source precedence and its comment are clear. One of the new
unit tests still cannot detect the distributed-before-history regression.
>
> Reviewed head `2b541892ec8d418da68469cab5b07bd4c5d40cfc` against base
`a3743d7f52e9200a7733857ca813a0836bf66bba`. Report time:
2026-11-06T01:19:33.596416+00:00.
>
> # What Problem Does This PR Solve?
> **User pain point:** Same-ID restore can expose the previous attempt
observability settings from finished history.
>
> **Fix approach:** Prefer running, pending and distributed current metadata
before finished history.
>
> **One-sentence summary:** Read current-attempt configuration when old
history shares the same job ID.
>
> **Example:** Restore job 42 with enabled=true and bucket_ms=2000 after
saving an attempt with enabled=false and bucket_ms=1000; realtime endpoints
should reflect the restored values.
>
> # 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-1958`
changes lookup precedence and reconstructs distributed metadata through the
serialized logical DAG.
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1511-1539`
adds the distributed precedence case, while
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/RealtimeMetricsRestIT.java:247-373`
covers real savepoint/restore endpoint smoke behavior.
>
> **Before / after:**
>
> ```
> Before: history -> running -> pending -> distributed
> After: running -> pending -> distributed -> history
> Distributed lookup: deserialize immutable information -> restore DAG ->
construct new JobDAGInfo
> ```
>
> **Key findings:**
>
> * Realtime collection and metadata endpoints use this lookup.
> * Active/pending attempts override old history.
> * The distributed-only submission/takeover window is also changed.
> * Mutating a returned reconstructed DAG does not update the serialized
distributed JobInfo.
> * The new distributed test expects the same false value that old
history-first logic returns.
>
> **Complete runtime path:**
>
> ```
> Savepoint stop -> retain history -> submit same ID -> distributed JobInfo
-> pending -> running
> RealtimeMetricsService.collectOnce -> resolveObservabilityConfig ->
CoordinatorService.getJobInfo
> running/pending DAG, otherwise deserialize distributed DAG, otherwise
history
> -> current enabled/bucket settings -> stores ->
jobs/edges/vertices/points REST
> ```
>
> **Correctness and boundaries:** The source order fixes the intended
current-attempt problem. During terminal cleanup a distributed DAG can outlive
the active master; reconstructing it can omit historical execution-plan
details, as the author explicitly describes. The history fallback remains
reachable after distributed removal. The distributed precedence test changes
only a detached return value, then asserts false from a fresh lookup, so it
passes whether history wins or distributed wins.
>
> ## 1.2 Compatibility Impact
> **Fully compatible.** No public API, option name, default, protocol or
serialized state is changed.
>
> ## 1.3 Performance / Side-Effect Analysis
> Active lookups avoid history access. Distributed-only lookups still
deserialize the DAG and now precede history, potentially doing more work in the
cleanup window. No new threads, locks or buffers are added; metrics-store reset
semantics, retries and checkpoint formats are unchanged.
>
> ## 1.4 Error Handling and Logging
> JobNotFoundException remains the absent-metadata result. No new log or
credential handling is introduced. This is not a fix for all stale
metric-window or cleanup races.
>
> ### Issue 1: Make the distributed precedence test distinguish the
competing metadata sources
> * **Location:**
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1523`
> * **Problem description:** getJobInfo reconstructs a fresh JobDAGInfo from
distributed serialized metadata. Changing that detached envOptions map to true
does not update runningJobInfoIMap. The mocked history is false, and the final
assertion also expects false, so history-first code passes.
> * **Potential risk:** The submission/master-takeover branch can regress
without this test failing.
> * **Best improvement:** Seed the actual distributed JobInfo with distinct
enabled/bucket values, retain different history values, and assert the
distributed values. Confirm the test fails if history is moved before
distributed lookup.
> * **Severity:** Medium (blocking)
> * **Raised by another reviewer:** No
>
> ### Issue 2: Explain the restore configuration and readiness helpers
> * **Location:**
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/RealtimeMetricsRestIT.java:383`
> * **Problem description:** The new helpers build a long-running restore
fixture and poll reflective task readiness, but do not document why those
preconditions matter.
> * **Potential risk:** Future test edits can remove the readiness guarantee
or shorten the source unintentionally.
> * **Best improvement:** Add short comments describing the same-ID restore
contract, long-lived fixture and readiness condition.
> * **Severity:** Medium (non-blocking)
> * **Raised by another reviewer:** No
>
> # 2. Code Quality Assessment
> ## 2.1 Coding Standards
> The core precedence comment resolves the earlier documentation request.
The new restoreObservabilityConfig and awaitAllTasksReady helpers should
explain their readiness/restore purpose (non-blocking Issue 2).
>
> ## 2.2 Test Coverage and Test Stability
> Test stability: **Stable**.
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/RealtimeMetricsRestIT.java:247-373`
uses condition-based Awaitility checks and temporary files removed in finally;
the long-lived source and sink delay prevent normal completion before
savepoint. Client/node cleanup remains in the existing teardown.
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1511-1539`
has a regression-power defect (Issue 1), not an intermittent timing defect.
The author correctly calls the cluster test smoke coverage because it also
passes under the old lookup order; that makes distinct unit fixtures essential.
>
> **Local verification:** `git fetch` obtained the reviewed head; an
independent `seatunnel-review-13136` worktree was checked out. `git diff
a3743d7f52e9200a7733857ca813a0836bf66bba...2b541892ec8d418da68469cab5b07bd4c5d40cfc`
covered all 5 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
> EN/ZH observability notes correctly scope this to restored-attempt
settings. No claim is made that all metrics stores are reset.
>
> # 3. Architectural Soundness
> ## 3.1 Elegance of the Solution
> **Precise fix.** The correction belongs at the affected boundary rather
than in unrelated engine code.
>
> ## 3.2 Maintainability
> Keep the source declarations as the reference for future changes.
>
> ## 3.3 Extensibility
> No additional abstraction is needed for this focused correction.
>
> ## 3.4 Historical-Version Compatibility
> Historical API, configuration and checkpoint formats are unchanged.
>
> # 4. Issue Summary
> Number Issue Location Severity
> 1 Make the distributed precedence test distinguish the competing metadata
sources
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:1523`
Medium
> 2 Explain the restore configuration and readiness helpers
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/RealtimeMetricsRestIT.java:383`
Medium
> Full-scan check: complete diff, affected callers, failure/lifecycle
branches, compatibility, test behavior/stability, documentation and current
merge gates examined. Existing reviews, inline comments and discussion were
read; there is no independent technical finding or maintainer
approval/change-request intent to reconcile.
>
> # 5. Merge Recommendation
> ### Conclusion: Ready to merge after fixes
> 1. **Blockers — must be fixed:** Issue 1: Make the distributed precedence
test distinguish the competing metadata sources No Apache Build check is
attached in the collected current-head snapshot. No exact-head fork Build was
found in the collected snapshot. Missing checks alone do not establish disabled
workflows. A write-capable maintainer owns workflow approval/check adoption and
the effective merge approval.
> 2. **Recommended fixes — non-blocking:** Issue 2: Explain the restore
configuration and readiness helpers
>
> **Overall assessment and alternatives:** Keep the implementation focused
and correct Issue 1. Coordinate the overlapping #13133 merge decision; neither
contributor should need to stack duplicate lookup fixes.
>
> Upstream snapshot: `dev`, ahead 2, behind 3, compare `diverged`, 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, @DanielLeens, good catch. Pushed 6f745fc24:
- Issue 1: the distributed precedence test now seeds a serialized JobInfo in
runningJobInfoIMap with enabled=true and bucket_ms=2000, while the mocked
history for the same job ID returns enabled=false and bucket_ms=1000, with no
running or pending JobMaster. It asserts the distributed values. With the
history lookup temporarily moved ahead of the distributed lookup it fails
(expected: <true> but was: <false>), and it passes with the current order. I
also checked the other new precedence tests: they use distinct values, and the
same-attempt and unchanged-config tests intentionally use equal values.
- Issue 2: added comments on restoreObservabilityConfig and
awaitAllTasksReady describing the same-ID restore contract, why the fixture is
long-lived, and what the readiness check guarantees.
CoordinatorServiceTest passes locally (44 run, 0 failures, 2 skipped). The
RealtimeMetricsRestIT changes in this commit are comments only. I couldn't run
test-compile for the e2e module locally because the dependency reactor fails in
an unrelated module (seatunnel-translation-flink-common, unresolved shaded
Config class), so I'm relying on CI for that.
The PR signal / Build check is waiting for workflow approval. Could a
maintainer please approve the 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]
