lizhimins commented on PR #4970:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4970#issuecomment-5806619928
This cannot be merged as it stands — it does not compile, and the diagnosis
it adds largely duplicates two endpoints that already exist.
1. `RocketMQConsumerDiagnosticsProvider` is `@RequiredArgsConstructor` with
three final fields. Adding a fourth (`ConsumerHangDiagnosticEngine`) changes
the generated constructor, but
`RocketMQConsumerDiagnosticsProviderTest.java:67` still calls `new
RocketMQConsumerDiagnosticsProvider(runtimeAdminClientResolver, adminFactory,
properties)` and this PR does not touch that file, so the test module no longer
compiles.
2. The consumer-side jstack is already fetched and parsed:
`RocketMQConsumerDiagnosticsProvider.java:86` calls
`admin.getConsumerRunningInfo(group, clientId, true)` and `parseJstack` (line
139+) turns it into `ConsumerThreadStackVO` rows including the thread state,
exposed as `GET /api/groups/{name}/instances/{clientId}/stack`; per-queue
progress is `GET /api/groups/{name}/progress`. A hang report that re-reads the
same `ConsumerRunningInfo` should extend those, not sit beside them.
3. `countBlockedThreads` counts every `java.lang.Thread.State: WAITING
(parking)` line and `blockedThreads > 2` alone flips the report to WARNING. A
healthy RocketMQ client parks its idle `PullMessageService`, `RebalanceService`
and scheduled pools, so this reports WARNING for normal consumers. Stall
duration is also computed as server `System.currentTimeMillis()` minus a
client-reported `lastConsumeTimestamp`, with no clock-skew handling — a skewed
client produces a false CRITICAL.
4. All five tests in `ConsumerHangDiagnosticEngineTest` are named `testXxx`;
the convention here is a `...Test` suffix. The provider-side refactor
(`resolveRunningInfo` / `fetchRunningInfo`) — the only change on a real code
path — has no test at all.
5. Please also add `GET
/api/groups/{name}/instances/{clientId}/hang-diagnostics` to
`docs/api-spec.md`, and use fully qualified type names nowhere: import
`ConsumerHangReportVO` / `ConsumerHangDiagnosticEngine` instead of inlining the
package in the field and return types.
A narrower version of this — reuse the existing running-info fetch, report
stall from queue progress rather than client timestamps, and derive
blocked-thread facts from the already-parsed `ConsumerThreadStackVO` states —
would be welcome.
--
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]
