DanielLeens commented on PR #11732:
URL: https://github.com/apache/seatunnel/pull/11732#issuecomment-5237301203
# What Problem Does This PR Solve?
This PR adds a design/contract document
(`docs/en|zh/engines/zeta/worker-node-resource-view.md`) for issue #11665,
proposing to turn the already-collected per-node
`/system-monitoring-information` data plus the resource manager's live
`WorkerProfile` state into a usable per-worker slot/resource view on the
existing Workers/Master pages, via one new read-only REST projection. It is a
docs-only change (no code): two new Markdown pages (EN/ZH), a sidebar
registration, and two small cross-links from `web-ui.md`.
**Note up front:** this PR is already closed by the author. On 2026-08-10,
`danielnadean` (the PR author) commented directly on issue #11665 withdrawing
this proposal in favor of `@goutamadwant`'s alternative backend contract, and
stated they were "closing #11732 / #11733." I verified that both the technical
objection and the withdrawal are accurate against the current `dev` source
(details below), so this review focuses on confirming that the self-withdrawal
is correct and documenting why, for the record, rather than pushing toward
merge.
# 1. Code Change Review
## 1.1 Core Logic Analysis
This is a pure documentation change — no production code, REST endpoints, or
UI components are touched. The diff:
- `docs/en/engines/zeta/worker-node-resource-view.md` (new, 121 lines) /
`docs/zh/...` (new, matching translation) — the design contract itself.
- `docs/en/engines/zeta/web-ui.md` / `docs/zh/...` — adds one sentence
pointing to the new design doc from the Workers section, plus a "Related Docs"
list entry.
- `docs/sidebars.js` — registers `engines/zeta/worker-node-resource-view` in
the Zeta engine sidebar, correctly nested next to `engines/zeta/web-ui` inside
the same category.
I cross-checked every concrete technical claim in the design doc against the
current `dev` source rather than taking the PR description's "Validation"
section at face value, since a design doc's whole value is being accurate about
the code it's proposing to build on:
| Claim in doc | Verified against | Result |
|---|---|---|
| Workers/Master page renders only 4 columns (Host, Port, Physical MEM
Total, Heap MEM Used); Action column exists but is commented out |
`seatunnel-engine/seatunnel-engine-ui/src/views/managers/index.tsx` | Accurate
— the `createColumns()` function defines exactly those 4 columns, and the
`Action`/`View` button block is commented out inline. |
| `/system-monitoring-information` returns "roughly 35 fields" via `Monitor`
| `seatunnel-engine/seatunnel-engine-ui/src/service/manager/types.ts` | Close
enough — the `Monitor` interface has ~45 fields including
`isMaster`/`host`/`port`/`processors`; "roughly 35" undercounts slightly but
the qualitative point (far more than the 4 rendered columns) holds. |
| `OverviewInfo` exposes cluster-wide `totalSlot`/`unassignedSlot` only |
`OverviewInfo.java` | Accurate — `private int totalSlot; private int
unassignedSlot;`, no per-worker breakdown. |
| `ResourceManager.getRegisterWorker()` returns `ConcurrentMap<Address,
WorkerProfile>` | `ResourceManager.java:74` | Accurate, exact signature match. |
| `WorkerProfile` carries `assignedSlots`/`unassignedSlots`
(`SlotProfile[]`), `dynamicSlot`, `attributes`, `systemLoadInfo` |
`WorkerProfile.java` | Accurate, exact field match. |
| `GetOverviewOperation` / `OverviewService` cross-node pattern
(`getSeaTunnelServer(true)` + `NodeEngineUtil.sendOperationToMasterNode`) |
`OverviewService.java` | Accurate. |
| `/trace/task-mapping/:jobId` is job-scoped | `RestConstant.java`,
`RestHttpGetCommandProcessor.java` | Accurate — the route is
`/trace/task-mapping/{jobId}`, no cluster-wide variant. |
One minor accuracy gap: the doc refers to the manager view as
`seatunnel-engine-ui/src/views/managers/index.tsx` in both language versions,
but the real path has a `seatunnel-engine/` prefix
(`seatunnel-engine/seatunnel-engine-ui/src/views/managers/index.tsx`).
Low-stakes for a design doc, but worth a fixup if this content is ever revived.
More importantly, on the substance: the design's core V1 mechanism —
```
totalSlot = assignedSlots.length + unassignedSlots.length
usedSlot = assignedSlots.length
```
— is exactly what the author flagged as wrong in their own issue-#11665
follow-up comment, and I confirmed it against `WorkerProfile.java` and the
broader resource-manager model: for dynamic-slot workers, `unassignedSlots` is
not a fixed remaining capacity pool the way it is for fixed-slot workers, so
summing the two array lengths does not represent true "total capacity." The
author also found that `WorkerResourceDiagnostic`
(`org.apache.seatunnel.engine.server.diagnostic.WorkerResourceDiagnostic`)
already models almost this exact shape — `address`, `tags`, `totalSlots`,
`freeSlots`, `dynamicSlot`, `cpuUsage`, `memUsage`, `runningJobIds` — which I
verified field-for-field against the current source. Building a parallel
`WorkerOverviewInfo` DTO with different field names for the same concepts,
without reconciling with `WorkerResourceDiagnostic` or the running-job-centric
slot API already in flight on PR #11597, would have created a third competing
slot-representation model. T
hat's a real, substantiated design flaw, not a nitpick, and the author's
self-correction is the right call.
## 1.2 Compatibility Impact
**Fully compatible.** Docs-only change; adds two new pages and two new
links. No config options, APIs, or serialization formats are touched. N/A given
the PR is withdrawn, but noted for completeness.
## 1.3 Performance / Side-Effect Analysis
None — no runtime code changes.
## 1.4 Error Handling and Logging
N/A — docs-only change, no error paths introduced.
No formal numbered issues in this section; the one substantive technical
problem (dynamic-slot `totalSlot` computation) is the reason the PR is
withdrawn and is discussed above rather than listed as a blocking "Issue N,"
since fixing it would mean redesigning the contract, not patching this diff.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Markdown formatting is clean and consistent with other docs in
`docs/en/engines/zeta/`. Both EN and ZH versions are structurally parallel
(same headings, same table shapes), which is good practice for this repo's
bilingual doc requirement. N/A for code comments since no source files are
touched.
## 2.2 Test Coverage and Test Stability
N/A — no UT/E2E code is touched by this PR, so no flaky-test-risk rating
applies. (The design doc itself proposes future test requirements — zero-worker
and zero-slot cases for backend tests, client-side join tests for frontend —
which is good practice to bake into a design contract before implementation
starts, and is consistent with this repo's testing expectations.)
## 2.3 Documentation Updates
Both `docs/en` and `docs/zh` are updated in lockstep, satisfying the repo's
bilingual documentation requirement. The cross-links from `web-ui.md` in both
languages, and the sidebar registration, are all internally consistent — I
verified every linked target file (`web-ui.md`, `rest-api-v2.md`,
`realtime-observability.md`) exists in the same directory.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
The doc's overall bounded-scope approach (reuse existing monitoring payload,
reuse existing `WorkerProfile` state, one new read-only projection, no new
scheduling state) is a sound shape for a V1 design. However, the specific
slot-accounting formula it proposes (`assignedSlots.length +
unassignedSlots.length`) does not hold for dynamic-slot workers, and the design
would have introduced a slot representation parallel to the existing
`WorkerResourceDiagnostic` and the in-flight PR #11597 rather than building on
either. This is exactly the kind of thing a design-doc review round is supposed
to catch before implementation starts, so the process worked as intended — it
just happened to be caught by the author re-reading their own proposal against
`@goutamadwant`'s alternative rather than by this review round.
## 3.2 Maintainability
N/A for the code path (none touched). As a piece of documentation, if this
content were resurrected it would need the dynamic-slot semantics reworked and
the `WorkerResourceDiagnostic` reuse question resolved before it's a safe
contract for someone to implement against.
## 3.3 Extensibility
N/A — moot given withdrawal.
## 3.4 Historical-Version Compatibility
N/A — docs-only, no version-sensitive surface touched.
# 4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Proposed `totalSlot = assignedSlots.length + unassignedSlots.length`
formula is incorrect for dynamic-slot workers (author-identified, verified
against `WorkerProfile.java`) |
`docs/en/engines/zeta/worker-node-resource-view.md` ("Per-Worker Slot State"
table), mirrored in `docs/zh/...` | High (design-correctness, already
acknowledged by author) |
| 2 | Design introduces a `WorkerOverviewInfo` DTO that duplicates the
existing `WorkerResourceDiagnostic` shape instead of reusing/aligning with it,
and doesn't reconcile with the in-flight slot-usage work on #11597
(author-identified) | `docs/en/engines/zeta/worker-node-resource-view.md` ("V1
Delivery Plan" step 1), mirrored in `docs/zh/...` | Medium (architectural
fragmentation risk, already acknowledged by author) |
| 3 | Referenced source path
`seatunnel-engine-ui/src/views/managers/index.tsx` is missing the
`seatunnel-engine/` prefix present in the actual repo path |
`docs/en/engines/zeta/worker-node-resource-view.md` ("Problem" section),
mirrored in `docs/zh/...` | Low (cosmetic, docs-only) |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
1. Blockers — must be fixed
- None from my own review — the PR is already closed by its author, who
withdrew it in favor of `@goutamadwant`'s alternative backend contract on issue
#11665 (comment
https://github.com/apache/seatunnel/issues/11665#issuecomment-5236826008). I
independently verified the two technical reasons for withdrawal (Issues #1 and
#2 above) against current `dev` source and they are both accurate and
substantive, not overcautious self-doubt.
2. Recommended fixes — non-blocking
- If any part of this content is salvaged for a future revision of the
design doc, fix the `seatunnel-engine-ui/...` path to include the
`seatunnel-engine/` prefix (Issue #3), and rework the "Per-Worker Slot State"
section to use `WorkerResourceDiagnostic`'s existing fixed/dynamic-slot
distinction rather than the flat `assignedSlots.length +
unassignedSlots.length` sum.
Overall assessment: this was a well-researched design doc — every concrete
technical claim I checked against the source (component paths, field names,
method signatures, REST route scoping) matched exactly, which reflects real
source-reading rather than guesswork. The one thing it got wrong (dynamic-slot
capacity accounting) is a legitimately subtle point, and the author caught it
themselves and did the right thing by withdrawing rather than pushing an
incorrect contract through review. No action needed here beyond confirming the
closure is well-founded; the follow-up work should proceed against
`@goutamadwant`'s proposal on #11665 instead.
---
*Note: this PR was closed by the author during this review round, so this is
posted as a regular comment rather than a formal review.*
--
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]
