featzhang commented on PR #27774:
URL: https://github.com/apache/flink/pull/27774#issuecomment-4362585810
## Self-review after a focused read of this PR
I went through the diff end-to-end and, besides the CI/docs fixes already
landed, there are still a number of correctness issues worth capturing so
they
don't fall through the cracks. I've split them by severity below and pushed
two follow-up commits (`daadac3`, `4a24d51`) that address the three
blockers (B1/B2/B3/B4/B5) — the rest are tracked here as TODOs for a
follow-up pass.
### Status legend
- ✅ fixed in this PR (commits `daadac3`, `4a24d51`)
- 📝 still open — tracked for a follow-up pass
---
### 🔴 Blockers
**✅ B1. Hard-coded metric names did not exist in Flink**
The handler referenced `taskmanager.cpu.time`, `tasks.backpressure.ratio`,
and
`tasks.GC.time`. A tree-wide grep confirmed none of these strings appear
anywhere else in the codebase. Rewired to:
- CPU → `Status.JVM.CPU.Load`
- Backpressure → `MetricNames.TASK_BACK_PRESSURED_TIME`
(`backPressuredTimeMsPerSecond`)
- GC → sum of `Status.JVM.GarbageCollector.<name>.Time` over all
collectors reported by the TaskManager.
**✅ B2. CPU / GC were read at the wrong scope**
CPU load and GC time are JVM-level metrics reported by the TaskManager, not
per-subtask metrics. The previous code walked
`taskMetricStore.getAllSubtaskMetricStores()` for both, which would have
produced nothing even after B1. CPU and GC now iterate
`MetricStore.getTaskManagers()` and the response's `subtaskId`/`operatorName`
fields are populated with TaskManager-scoped placeholders (-1 and
"TaskManager" respectively).
**✅ B3. Frontend was wired to mock data — the service was never
called**
Three components (`OverviewComponent`, `ApplicationOverviewComponent`,
`JobTopnMetricComponent`) each contained an `initTopNMetricsData()` method
that hard-coded identical "Kafka Source / 95.5 / container_123" rows, and
`TopNMetricsService.loadTopNMetrics` was defined but never injected anywhere.
`JobTopnMetricComponent` now injects `TopNMetricsService` + `StatusService`
and polls the real endpoint on every refresh tick. The two cluster-level
pages drop the mock block entirely; Top-N is inherently job-scoped and can't
be meaningfully populated without a chosen job, so the component renders its
empty state there until cluster-aware aggregation is added.
**✅ B4. `taskManagerId` was the literal string `"unknown"`; `operatorName`
re-used `taskId`**
Both are the primary way operators identify a hot component. The handler now
propagates the real TaskManager ID and uses sensible placeholders only where
the scope is genuinely not per-subtask (CPU / GC at the TM level).
**✅ B5. `representativeAttempts` was used as the only vertex enumerator**
`representativeAttempts` is an internal structure tracking representative
attempt numbers, and is empty before `updateCurrentExecutionAttempts` has
fired. The handler now guards on `metricStore.getJobMetricStore(jobId) ==
null`
and returns early, which also documents the semantic intent.
---
### 🟠 Major — still open
**📝 M1. No tests**
`find flink-runtime/src/test -name 'TopNMetrics*'` returns nothing. Peer
handlers all have unit tests that stub a `MetricStore`/`MetricFetcher` and
assert the response shape — please add at minimum a
`TopNMetricsHandlerTest` covering Top-N ordering, `limit`, and empty-metric
fallbacks, plus snapshot coverage for `TopNMetricsResponseBody` shape.
**📝 M2. `DEFAULT_TOP_N = 5` is not configurable**
The endpoint is named "Top N" but exposes no way for the client to change N.
Adding an optional `topN` query parameter (with a sane upper bound like 100
to
avoid accidental DoS) would be a very small change and unblocks runner-up
investigation in large jobs.
---
### 🟡 Minor — still open
**📝 N1. Field names vs. semantics mismatch**
- `CpuConsumerInfo.cpuPercentage` was named to suggest a percentage, but the
previous implementation stored raw CPU time. The follow-up commit now
writes the actual percentage (0–100) into this slot, but the field
name no longer matches the GC slot on the same response body —
`GcTaskInfo.gcTimePercentage` is now cumulative GC **milliseconds**, not a
percentage. Both names are load-bearing in the wire protocol, so renaming
them is a breaking change; worth a follow-up PR that either renames or
normalizes both to percentages (the GC side would need a denominator, e.g.
uptime, to compute a meaningful ratio).
**📝 N3. Unrelated formatting churn in two files**
`flink-runtime-web/web-dashboard/src/app/pages/job-manager/logs/job-manager-logs.component.ts`
(+11/-13) and the task-manager variant (+13/-15) are pure import reformatting
unrelated to this feature. Please consider moving those to a separate
cleanup PR to keep the diff focused.
**📝 N4. `package-lock.json` has `+1666/-2933`**
That's essentially a full re-lock, and it's not obvious whether the
accompanying `package.json` changes justify the drift. Worth confirming the
lock matches `package.json` exactly and isn't picking up unrelated dependency
bumps.
**📝 N5. Empty-state rendering**
`<div *ngIf="items.length===0">` below an empty `nz-table` could be replaced
with the table's built-in `nzNoResult` slot for tighter layout.
**📝 N7. `TopNMetricsMessageParameters` is currently empty**
It just extends `JobMessageParameters` with no additional parameters. Either
drop it in favour of `JobMessageParameters` directly, or keep the class name
as the natural extension point once M2 introduces `topN` as a query
parameter.
**📝 N8. Commit title module name**
`[Web Dashboard]` is non-standard; the module name convention elsewhere is
`[flink-runtime-web]` or `[web]`. The follow-up commits adopt
`[flink-runtime-web]` and `[rest]`.
---
### ✅ What looks good
- REST endpoint registration and `RuntimeMessageHeaders` wiring follow the
established pattern in `WebMonitorEndpoint`.
- `TopNMetricsResponseBody` uses `@JsonCreator` + `@JsonProperty` +
`FIELD_NAME_*` constants and matches sibling response bodies.
- The component is `standalone` + `OnPush`, consistent with the dashboard
rewrite.
- REST API docs (`rest_v1_dispatcher.html`/`.yml`) and
`rest_api_v1.snapshot` were already regenerated in the earlier commits,
and since the follow-up rewire keeps the wire contract stable they do not
need regenerating again.
Happy to iterate on M1/M2/N1 next if reviewers agree with the direction.
--
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]
