DanielLeens commented on PR #11733:
URL: https://github.com/apache/seatunnel/pull/11733#issuecomment-5237301511
# What Problem Does This PR Solve?
The Zeta Web UI's "Workers"/"Master" pages currently show only 4 raw fields
from `/system-monitoring-information` (host, port, physical memory, heap used)
and no per-worker slot/capacity view, even though the resource manager already
tracks per-worker slot assignment
(`WorkerProfile#assignedSlots`/`unassignedSlots`) for scheduling. This PR adds
a new read-only `GET /resource/workers` endpoint that projects that existing
state into a `WorkerOverviewInfo` DTO (host, port, totalSlot, usedSlot,
dynamicSlot, cpuPercentage, memPercentage, attributes), joins it client-side
with the existing monitoring table by `host:port`, and adds a details drawer
("View") reusing the existing `Configuration` component. Docs (`web-ui.md`,
`rest-api-v2.md`, EN/ZH) are updated accordingly.
**Important context that changes the shape of this review:** the PR is
currently **closed**, and the author (danielnadean) posted a withdrawal comment
on the PR itself:
> "Superseded - withdrawing in favor of @goutamadwant's proposal on #11665
(correctly handles dynamic-slot semantics and reuses/aligns with the existing
WorkerResourceDiagnostic and #11597 instead of adding a third slot model)."
On the parent issue (#11665), the author further details this: their own
`totalSlot = assignedSlots.length + unassignedSlots.length` computation is
acknowledged to be wrong for dynamic-slot workers, and there is an existing
`WorkerResourceDiagnostic` class
(`org.apache.seatunnel.engine.server.diagnostic`) modeling almost the same
per-worker shape (`totalSlots`, `freeSlots`, `dynamicSlot`, `cpuUsage`,
`memUsage`) that this PR duplicates with different field names instead of
reusing.
Given that, I want to be upfront: I have never reviewed this PR before, so
this is not a "the last round missed something" situation — but since the PR is
already closed and the author has self-diagnosed the main design problem, this
review's job is mainly to (a) independently confirm the author's own findings
against the actual source so the reasoning is on record for whoever picks up
the follow-up (@goutamadwant's proposal), and (b) surface a few additional,
purely mechanical issues I found while verifying the diff, in case any of this
code/tests gets reused going forward.
---
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Runtime path traced from source (all confirmed by reading the current
head, not just the PR description):**
```
GET /resource/workers
-> WorkerOverviewServlet#doGet
-> WorkerOverviewService#getWorkerOverviewInfos
- if this node is master (BaseService#getSeaTunnelServer(true)):
GetWorkerOverviewOperation.getWorkerOverviewInfos(server)
[local, same thread]
- else:
NodeEngineUtil.sendOperationToMasterNode(nodeEngine, new
GetWorkerOverviewOperation()).join()
[forwarded via Hazelcast operation service to the master node]
-> GetWorkerOverviewOperation.getWorkerOverviewInfos(server)
-> ResourceManager#getRegisterWorker()
(AbstractResourceManager#registerWorker, a plain
ConcurrentHashMap<Address, WorkerProfile> already populated by the
existing worker
registration/heartbeat path)
-> GetWorkerOverviewOperation#toWorkerOverviewInfo(WorkerProfile) for
each entry
-> WorkerOverviewInfo{host, port, usedSlot=assignedSlots.length,
totalSlot=assignedSlots.length+unassignedSlots.length,
dynamicSlot,
cpuPercentage/memPercentage from SystemLoadInfo, attributes}
```
This is a correctly-wired, read-only projection: `getRegisterWorker()` is an
existing `@Getter`-exposed `ConcurrentMap` (`AbstractResourceManager.java:60`),
no new mutable state is introduced, and the local-vs-forward-to-master routing
mirrors `OverviewService` exactly (`WorkerOverviewService.java` vs
`OverviewService.java`). `WorkerOverviewService` even adds a defensive
`nodeEngine.getMasterAddress() == null` check before forwarding that
`OverviewService` lacks — a minor, harmless robustness improvement, not a
behavior change for the existing endpoint.
**The core correctness problem (confirmed independently, matches the
author's own admission):**
`GetWorkerOverviewOperation.java:306-315` computes:
```java
int assignedSlots = workerProfile.getAssignedSlots() == null ? 0 :
workerProfile.getAssignedSlots().length;
int unassignedSlots = workerProfile.getUnassignedSlots() == null ? 0 :
workerProfile.getUnassignedSlots().length;
info.setUsedSlot(assignedSlots);
info.setTotalSlot(assignedSlots + unassignedSlots);
```
This mirrors `GetOverviewOperation.getOverviewInfo`
(`GetOverviewOperation.java:91`,
`overviewInfo.setTotalSlot(assignedSlots.size() + unassignedSlots.size())`) —
so it is not a new bug pattern invented by this PR, it's the same "total =
assigned + unassigned array lengths" convention already used for the
cluster-wide `/overview` endpoint. However, for **dynamic-slot workers**
(`WorkerProfile#dynamicSlot == true`), slots are created from remaining
resource on demand rather than pre-allocated into a fixed `unassignedSlots`
pool, so `unassignedSlots.length` does not represent "remaining capacity" the
way it does for fixed-slot workers. Presenting `totalSlot = usedSlot +
unassignedSlots.length` for a dynamic-slot worker is therefore misleading (as
the author agrees) — it either always shows `total == used`, or shows a number
that has no real capacity meaning, depending on what `unassignedSlots` happens
to contain for the dynamic case. This is exactly the finding @goutamadwant rai
sed on #11665 and the author accepted.
**Model duplication:** `WorkerOverviewInfo` (new, this PR) and
`WorkerResourceDiagnostic` (existing,
`org.apache.seatunnel.engine.server.diagnostic.WorkerResourceDiagnostic`) both
represent "per-worker slot/load snapshot" with different field names
(`totalSlot`/`usedSlot` vs `totalSlots`/`freeSlots`,
`cpuPercentage`/`memPercentage` vs `cpuUsage`/`memUsage`).
`WorkerResourceDiagnostic` is currently only used from the pending-diagnostics
collector path (`PendingDiagnosticsCollector`, `PendingClusterSnapshot`), not
exposed over REST, but the two DTOs modeling the same concept independently is
the kind of fragmentation that should be reconciled before a REST-facing v1
ships, especially with #11597 also adding job-centric slot usage in parallel.
**Broken doc link introduced by this diff:**
`docs/en/engines/zeta/web-ui.md` and `docs/zh/engines/zeta/web-ui.md` both add
a link to `worker-node-resource-view.md` ("See [Worker Node Resource
View](worker-node-resource-view.md) for the design background."), but no such
file exists anywhere in this diff or in the base tree (`find docs -iname
"worker-node-resource-view*"` returns nothing). This correlates with the fork
CI's `Dead links` job reporting `failure` on this exact head SHA (job id
`93340292263`, run `31350524694`); I wasn't able to pull the raw job log
(network error fetching the Azure blob log URL from this environment), but the
missing target file is independently verifiable straight from the diff, and the
referenced design doc lives in the separate, now-closed PR #11732 which this
PR's description says should be merged first — it never was.
## 1.2 Compatibility Impact
**Fully compatible.** New REST endpoint (`GET /resource/workers`), new
servlet, new `GetWorkerOverviewOperation`/`GET_WORKER_OVERVIEW_TYPE = 13` (next
unused id in `ResourceDataSerializerHook`, no collision with existing 0-12),
new DTO, new frontend type/call. No existing config option, endpoint,
serialization id, or default value is touched. The frontend gracefully degrades
(`managerService.getWorkerOverview().catch(() => [])`) if the new endpoint is
unreachable, so an old-worker/new-master (or vice versa) rolling upgrade
wouldn't crash the UI, it would just show `-` in the new Slots column —
acceptable given this is a purely additive, non-critical UI feature.
## 1.3 Performance / Side-Effect Analysis
- The new call adds one extra `Promise`
(`managerService.getWorkerOverview()`) per page load, run in parallel with the
existing monitoring call (`Promise.all`), so no added latency on the happy path.
- Server-side, `getRegisterWorker()` is an in-memory map read with a
`.stream().map().collect()` over currently-registered workers — cheap, no new
locks, no polling loop.
- No new background thread, retry logic, or resource that could leak.
## 1.4 Error Handling and Logging
### Issue 1
- **Location:**
`seatunnel-engine/seatunnel-engine-ui/src/tests/managers.spec.ts:76-81` (and
the corresponding component logic in
`seatunnel-engine/seatunnel-engine-ui/src/views/managers/index.tsx:38,63-64,79-91`)
- **Problem:** The test's new assertion
`expect(wrapper.text()).toContain('2/4')` appears to be unreachable given the
component's actual runtime behavior. `managers/index.tsx` calls `useRoute()`
(line 38) and computes `isMaster = route?.path.endsWith('/master') || false`,
then filters `monitorRes` to `row.isMaster === String(isMaster)` before mapping
in the worker-overview join. `managers.spec.ts` mounts the component with only
`plugins: [i18n]` — no router plugin/mock is installed, unlike the sibling test
`detail.spec.ts` (`src/tests/detail.spec.ts:32-34`), which explicitly does
`vi.mock('vue-router', () => ({ useRoute: () => ({...}), ... }))` for exactly
this reason. Without that mock, `useRoute()` returns `undefined` under
vue-test-utils' `mount`, so `route?.path...` short-circuits to `undefined`, and
`isMaster` defaults to `false`. That means the filter keeps only `mockData[1]`
(`isMaster: 'false'`, port `5802`), which has **no** matching entry in
`mockWorkerOverview` (which
only defines port `5801`) — so the rendered table would show a single row
with the Slots column as the fallback `'-'`, and the row that would render
`'2/4'` (port `5801`, `isMaster: 'true'`) is filtered out entirely before the
join even matters.
- **Potential risk:** As written, this assertion looks like it would fail if
actually executed — but it never gets the chance to: the CI job that runs `npm
run test:unit` (`Build SeaTunnel UI` / `seatunnel-ui` job in
`.github/workflows/backend.yml:431-433`) is gated by `if:
needs.changes.outputs.api == 'true'` (`backend.yml:432`), and the `api`
path-filter only covers `seatunnel-api/**`, `seatunnel-common/**`,
`seatunnel-core/**`, etc. (`backend.yml:143`) — it does **not** include
`seatunnel-engine/**` or `seatunnel-engine-ui/**`. This PR only touches those
two paths, so the job is `skipped` on this exact head SHA (confirmed via the
fork's check-runs: `Run / Build SeaTunnel UI | completed | skipped`). In other
words, the new/modified frontend unit test is never executed by this PR's own
CI run, so its correctness is currently unverified by any automated gate.
- **Best improvement:** Add the same `vi.mock('vue-router', ...)` pattern
used in `detail.spec.ts` (mocking `path` to end in `/workers` or `/master` as
appropriate for the scenario under test), so the filter behavior is
deterministic and the assertions actually exercise the intended row.
- **Severity:** High (the test as written does not appear to validate what
it claims to, and no CI job currently catches that for engine/UI-only changes).
- **Raised by another reviewer:** No — the author's own withdrawal comment
on this PR does not mention this specific test issue; it's a separate,
independent finding on top of the design-level dynamic-slot issue the author
already flagged.
### Issue 2
- **Location:** `docs/en/engines/zeta/web-ui.md:68`,
`docs/zh/engines/zeta/web-ui.md:69`, `docs/en/engines/zeta/web-ui.md:87`,
`docs/zh/engines/zeta/web-ui.md:88`
- **Problem:** Both docs link to `worker-node-resource-view.md`, a file that
does not exist anywhere in this diff or the base tree.
- **Potential risk:** Broken link ships in user-facing documentation;
correlates with the fork's `Dead links` check reporting `failure` on this head
SHA.
- **Best improvement:** Either add the missing design doc (and keep it in
sync with wherever the design ultimately lands, since PR #11732 that was
supposed to add it is also closed), or drop the link until a design doc
actually exists.
- **Severity:** Medium (docs-only breakage; note the `Dead links` CI job has
`continue-on-error: true` per `backend.yml:112`, so this would not have blocked
merge on its own, but it's still a real broken link worth fixing).
- **Raised by another reviewer:** No.
---
# 2. Code Quality Assessment
## 2.1 Coding Standards
New Java classes (`GetWorkerOverviewOperation`, `WorkerOverviewInfo`,
`WorkerOverviewService`, `WorkerOverviewServlet`) all carry class-level Javadoc
explaining scope and constraints ("read-only projection... introduces no new
persisted or mutable state"), which is good practice given this codebase's
guidance to document lifecycle/runtime-path classes. The one non-trivial
private/package-private method, `toWorkerOverviewInfo`, has an inline comment
explaining why it's package-private (`// package-private for direct unit
testing without mocking the SeaTunnelServer chain`) — appropriate.
`WorkerOverviewService` and `WorkerOverviewServlet` lack per-field/per-method
Javadoc beyond the class-level comment, but their methods are short,
single-purpose, and named the same way as their
`OverviewService`/`OverviewServlet` counterparts, which is acceptable given the
existing convention in this package. `WorkerOverviewInfo`'s fields are simple
typed DTO fields without individual field comme
nts; given the class-level Javadoc already explains the DTO's purpose and each
field name is self-describing (`host`, `port`, `totalSlot`, ...), I would not
block on this, but a one-line comment on `cpuPercentage`/`memPercentage`
clarifying they're `Double` (nullable) specifically because a
freshly-registered worker may not have reported `SystemLoadInfo` yet would
remove any ambiguity for future readers.
## 2.2 Test Coverage and Test Stability
**Backend:** `GetWorkerOverviewOperationTest` (new) covers the pure mapping
function `toWorkerOverviewInfo` with two cases — a fully-populated
`WorkerProfile` and one with null slot arrays/load info (verifying
zero-instead-of-throw). Both cases use the real
`WorkerProfile`/`SlotProfile`/`ResourceProfile` constructors correctly
(verified against current signatures) and should compile and pass. However,
neither test case exercises a **dynamic-slot** worker, which is precisely the
scenario the author's own withdrawal identifies as broken — a gap directly
relevant to the PR's actual defect.
**Frontend flaky-test-risk rating: Risk present.** See Issue 1 above —
`managers.spec.ts`'s new `toContain('2/4')` assertion appears logically
unreachable given the component's routing-dependent filter and the absence of a
`vue-router` mock, and the CI job that would run it (`Build SeaTunnel UI`) is
skipped for this PR's file set (`api` path-filter doesn't cover
`seatunnel-engine/**`/`seatunnel-engine-ui/**`). This isn't "flaky" in the
intermittent-failure sense — it looks deterministically wrong — but it is
unverified, which for this review's purposes I'm treating the same way: don't
trust it as passing evidence without fixing the router mock.
## 2.3 Documentation Updates
`docs/en/engines/zeta/rest-api-v2.md` and `docs/zh/.../rest-api-v2.md` both
get a new endpoint section with matching parameter/response shape; `web-ui.md`
(EN/ZH) get matching updates to the Workers/Master feature descriptions and
table. Content stays in sync between EN and ZH. The one gap is the dangling
`worker-node-resource-view.md` link covered in Issue 2.
---
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
The read-only-projection approach and REST wiring pattern (mirroring
`OverviewService`/`OverviewServlet`) is the right shape for this kind of
feature and is not a "workaround" — it's a clean, minimal addition to an
existing, well-understood pattern. The specific `totalSlot` computation is
where it falls short, per the author's own analysis: it's a precise fix for
fixed-slot workers but an incorrect one for dynamic-slot workers, and it
introduces a DTO that duplicates `WorkerResourceDiagnostic`'s shape rather than
building on it.
## 3.2 Maintainability
Straightforward to maintain in isolation, but two competing per-worker slot
DTOs (`WorkerOverviewInfo` here, `WorkerResourceDiagnostic` existing) with
different field names for the same underlying concepts is a maintenance hazard
going forward — future changes to slot semantics would need to be kept in sync
across both, or one becomes stale. This is exactly the reason the author gave
for withdrawing.
## 3.3 Extensibility
The client-side `host:port` join pattern (`overviewByAddress` map in
`managers/index.tsx:774-777`) is a reasonable, extensible way to combine two
independently-sourced per-node datasets without server-side coupling, and the
`WorkerRow` type's `Partial<...>` fields correctly model that the two sources
can be out of sync (a monitored node without a matching resource-manager
registration, or vice versa).
## 3.4 Historical-Version Compatibility
No historical/serialization compatibility concerns: this is a purely
additive REST endpoint and DTO, no existing wire format, config option, or
persisted state is touched or reinterpreted.
---
# 4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | New `managers.spec.ts` assertion (`toContain('2/4')`) appears
unreachable given routing-dependent row filtering with no `vue-router` mock,
and the CI job that would run it is skipped for engine/UI-only diffs |
`seatunnel-engine/seatunnel-engine-ui/src/tests/managers.spec.ts:76-81` | High |
| 2 | Docs link to a design doc (`worker-node-resource-view.md`) that does
not exist in this diff or the base tree; correlates with the fork's `Dead
links` CI failure | `docs/en/engines/zeta/web-ui.md:68,87`,
`docs/zh/engines/zeta/web-ui.md:69,88` | Medium |
| 3 (design, self-identified by author) | `totalSlot = assignedSlots.length
+ unassignedSlots.length` is misleading for dynamic-slot workers, since
`unassignedSlots` isn't a fixed remaining-capacity pool in that mode |
`GetWorkerOverviewOperation.java:306-315` | High |
| 4 (design, self-identified by author) | New `WorkerOverviewInfo` DTO
duplicates the existing `WorkerResourceDiagnostic` concept with different field
names instead of reusing/aligning with it |
`resourcemanager/resource/WorkerOverviewInfo.java` vs
`diagnostic/WorkerResourceDiagnostic.java` | Medium |
---
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
1. **Blockers — must be fixed:**
- The PR is already **closed**, and the author has explicitly withdrawn
it in favor of @goutamadwant's proposal on #11665, which is scoped to correctly
handle dynamic-slot semantics and reconcile with `WorkerResourceDiagnostic` and
#11597 instead of introducing a third, competing slot model (Issue 3, Issue 4
above — both self-identified by the author, and independently confirmed against
source in section 1.1).
- If any part of this implementation is reused in the follow-up work, the
frontend test gap (Issue 1) and the broken doc link (Issue 2) should be fixed
before that follow-up ships, since neither is currently caught by CI for this
file set.
2. **Recommended fixes — non-blocking (for whoever picks up the follow-up
issue):**
- Reuse/align with `WorkerResourceDiagnostic`'s field naming and
dynamic-slot representation rather than introducing a parallel DTO.
- Add a `vi.mock('vue-router', ...)` to any future
`managers.spec.ts`-style test, following the existing `detail.spec.ts` pattern,
so route-dependent filtering is actually exercised.
- Keep the design-doc link only once the referenced doc actually exists,
or land both PRs together.
**Overall assessment:** The mechanical wiring in this PR (servlet
registration, serializer hook id, master-forwarding routing, DTO plumbing) is
solid and closely follows existing, proven patterns in this codebase — that
part is a good template to build on. But the core per-worker slot semantics are
wrong for dynamic-slot workers, and the DTO duplicates existing modeling, both
of which the author has already acknowledged and is the direct cause of
withdrawing this PR. Combined with an unverified frontend test and a docs link
to a file that doesn't exist, this PR in its current form shouldn't be merged
as-is — which matches where it already stands (closed). This review is intended
to leave a clear, source-verified record for whoever implements the follow-up
under @goutamadwant's proposal.
---
*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]
