Wang1rrr opened a new issue, #4851: URL: https://github.com/apache/rocketmq-dashboard/issues/4851
### Before Creating the Bug Report - [x] I have searched the [open issues](https://github.com/apache/rocketmq-dashboard/issues) of this repository and believe that this is not a duplicate. - [x] This is a defect in RocketMQ Studio, not a usage question and not a defect in another Apache RocketMQ repository. - [x] I can reproduce this on the current `rocketmq-studio` branch (the Studio trunk the pull request template points at), commit `4c697f07acde460e2344375cb1f82669f5b270fd`. ### Studio Version ``` branch: rocketmq-studio git commit id: 4c697f07acde460e2344375cb1f82669f5b270fd deployed as: built from source ``` ### Runtime Environment ``` OS: Windows 11 (development box) MySQL: not required - this is a static reachability and mapping defect, confirmed against the bytecode of rocketmq-tools 5.5.0 browser (for UI issues): n/a; no request reaches this code on any page ``` ### Connected RocketMQ Cluster ``` RocketMQ version: 5.5.0 (rocketmq-tools 5.5.0 on the Studio classpath) access mode: n/a - the code in question is never invoked for any access mode deployment: n/a ``` ### Describe the Bug `RocketMQMessageProvider` carries a 41-line private fallback for building a message's consumer status from the broker instead of from the trace topic: - `fallbackConsumerStatus(DefaultMQAdminExt, MessageExt)` at `provider/apache/RocketMQMessageProvider.java:811`, which calls `adminExt.messageTrackDetail(message)`; - `mapTrackType(TrackType)` at `:832`, its only helper. **Neither is reachable.** `fallbackConsumerStatus` has no caller anywhere in `server/src` (main or test), and `mapTrackType` is called only from inside it. The trace path builds its consumer status exclusively from parsed trace-topic records via `buildConsumerStatus` (`:777`). The method was already caller-less when it was introduced in `dc49e1a2` (feat: manage topics, groups, messages and DLQ against a live cluster, #798) and was carried through unchanged by `ceef946f` (refactor: unify multi-vendor architecture and backend code style, #1231). That matters because the unreachable code is also **wrong**, and it reads as if it were live: ```java private DeliveryStatus mapTrackType(TrackType trackType) { if (trackType == null) { return DeliveryStatus.pending; // "we could not tell" -> pending } switch (trackType) { case CONSUMED: case CONSUME_BROADCASTING: case CONSUMED_BUT_FILTERED: return DeliveryStatus.success; case NOT_CONSUME_YET: case PULL: case NOT_ONLINE: return DeliveryStatus.pending; default: return DeliveryStatus.failed; // <- TrackType.UNKNOWN lands here } } ``` `TrackType` has seven members in rocketmq-tools 5.5.0 (`CONSUMED, CONSUMED_BUT_FILTERED, PULL, NOT_CONSUME_YET, NOT_ONLINE, CONSUME_BROADCASTING, UNKNOWN`), so `UNKNOWN` is the only value that reaches `default` - and it does not mean "delivery failed". `DefaultMQAdminExtImpl.messageTrackDetail(MessageExt)` assigns `TrackType.UNKNOWN` as the **initial** value for every group it iterates, before it asks the broker anything, and only overwrites it with `NOT_ONLINE` when `examineConsumerConnectionInfo` fails with response code 206 (`CONSUMER_NOT_ONLINE`). Any other failure - unreachable broker, timeout, `RemotingException` - leaves the track at `UNKNOWN`. So `UNKNOWN` means "the admin query for this group did not complete", which is the same epistemic state as the `null` the method maps to `pending` two lines earlier. Two spellings of "we could not tell", two different answers, and one of them is the strongest negative claim the enum can make. If this fallback were ever wired in as written, `ConsumerStatusVO.deliveryStatus` would carry `failed`, and `DELIVERY_STATUS_MAP` (`web/src/pages/instance/message.tsx:108`) renders `failed` as `common.failure` (失败) in red - on the trace page of exactly those clusters where the admin RPCs are struggling, i.e. precisely when the operator is least able to tell a real delivery failure from a monitoring gap. And it would fire for *every* group of the topic, because `UNKNOWN` is the per-group starting value. For completeness, the live path is fine and is not part of this report: `buildConsumerStatus` reads `group = f[8]`, `isSuccess = f[4]`, `consumeTime = f[7]` from a `SubAfter` record, which matches `TraceDataEncoder.decoderFromTraceDataString` in rocketmq-client 5.5.0 field for field (0 type, 1 requestId, 2 msgId, 3 costTime, 4 isSuccess, 5 keys, 6 contextCode, 7 timeStamp, 8 groupName - the official decoder guards indices 6-8 the same way the comment at `:762` describes). ### Steps to Reproduce 1. `rg -n "fallbackConsumerStatus|mapTrackType|messageTrackDetail" server/src` - three hits, all inside the two methods themselves. 2. Confirm the mapping: `TrackType.UNKNOWN` is not listed in any `case`, so it falls to `default`. 3. Confirm what `UNKNOWN` means upstream: `javap -c org.apache.rocketmq.tools.admin.DefaultMQAdminExtImpl` and read `messageTrackDetail` - offset 66 sets `TrackType.UNKNOWN` on the fresh `MessageTrack` for each group, before `examineConsumerConnectionInfo` at offset 78, and offset 101 replaces it with `NOT_ONLINE` only for response code 206. ### What Did You Expect to See? Either the fallback is reachable and its statuses mean what they say, or it is not part of the codebase. An unreachable helper that encodes a wrong mapping is the worst of the three: it looks like implemented behaviour to the next reader, and it is the one a future change is most likely to wire up without re-reading `messageTrackDetail`. ### What Did You See Instead? 41 lines of private code that no request can reach, mapping the broker's "I could not determine this" to the console's red 失败. ### Additional Context Suggested fix, and the one in the pull request I am about to open: delete both methods and the two imports that become unused (`org.apache.rocketmq.tools.admin.api.MessageTrack`, `TrackType`). Checkstyle's `UnusedImports` runs in the `validate` phase of this build, so a partial removal fails the build - the deletion has to be complete. If the fallback is actually wanted, it should be a deliberate change rather than an accidental one, and two things need deciding first: 1. `TrackType.UNKNOWN` must map to `DeliveryStatus.pending`, matching the `null` case in the same method and the `pending` fallback `AliyunConverters` already uses for a status it cannot classify (`:327`); 2. the cost - `messageTrackDetail` issues one `queryTopicConsumeByWho` plus one `examineConsumerConnectionInfo` **per subscribed group**, and the natural trigger point is the `TOPIC_NOT_EXIST` / `NO_MESSAGE` branch at `:600`, i.e. the slow path of a page that is already blocked on RPCs. That may well be why it was never connected. Wiring it also needs the `MessageExt`: `resolveMessageStoreTimestamp` (`:670`) already fetches it via `viewMessage` / `viewMessageByOffsetId` and throws it away, keeping only the timestamp. Happy to send that as a separate proposal if the community wants the broker-side consumer status when the trace topic has nothing. ### Are You Willing to Submit a Pull Request? - [x] Yes, I am willing to submit a pull request. -- 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]
