zjncs commented on PR #4592: URL: https://github.com/apache/rocketmq-dashboard/pull/4592#issuecomment-5903950854
Thanks for the careful review — answering the four points in order. ### 1. The observable defect, end to end Both entry points drive the same service method (`DLQService.resendMessages` → `resendMessages(instanceId, group, start, end, target)`): the DLQ page's resend button (`POST /api/dlq/resend`) and the `rmq.message.redelivery_dlq` tool (`MessageRedeliveryDlqToolHandler`). Take a group whose `%DLQ%group` topic has two queues, one of which stalls (or hits persistent `OFFSET_ILLEGAL`) while the other scans cleanly with 40 dead letters in the window: - **Before this PR**: `failedQueueCount=0`, `truncated=false` → `scanIncomplete()=false` → `classifyOutcome` returns `SUCCESS`. The HTTP response / tool result reports `outcome=SUCCESS, matched=40, resent=40, scanIncomplete=false, failedQueueCount=0`, and the audit line records `RESEND_DLQ ... scanIncomplete=false, scanTruncated=false, scanFailedQueues=0` with result `SUCCESS`. The operator (or the agent consuming the tool) concludes every dead letter in the window was redelivered; whatever sits on the abandoned queue past the stall point was never read and never resent, with nothing in the response or the audit trail hinting at it. - **After this PR**: the same scenario reports `outcome=PARTIAL, scanIncomplete=true, failedQueueCount=1` — the UI badge, the tool result, and the audit line all say part of the DLQ was not covered. ### 2. How the two triggers happen in practice Honest answer: I reasoned these out from the code paths and the pull-consumer contract; I have not observed either against a production deployment, so I can't attach broker logs or name a version. - **Offset stall (`nextBeginOffset <= offset`)**: the loop advances by trusting `nextBeginOffset`. A `FOUND` pull whose `nextBeginOffset` equals the requested offset (or rewinds below it — e.g. a corrected next offset lower than the expired one we asked for after consume-queue compaction) makes no progress and would spin forever, so the guard breaks out with the remainder of that queue unscanned. - **Persistent `OFFSET_ILLEGAL`**: the common case is already handled by retrying from the corrected offset in `nextBeginOffset` (covered by `resendMessagesRetriesFromCorrectedOffsetAfterOffsetIllegal`). Reaching the give-up path needs the correction itself to keep being rejected — e.g. min/max offsets moving faster than the scan during heavy compaction, or a consume-queue inconsistency where every corrected offset still lands outside `[minOffset, maxOffset]`. `MAX_CONSECUTIVE_OFFSET_ILLEGAL=3` caps the retries, then the queue is abandoned. Both are broker-side edge conditions rather than everyday paths — which is what makes the misreport nasty: it fires precisely when the broker is already unhealthy (compaction pressure, queue anomalies), i.e. when the dead letters matter most. ### 3. Why reuse `failedQueueCount` rather than a distinct signal I kept the single counter deliberately, for three reasons: - **For the resend outcome the two facts are the same fact.** Whether a queue never returned a pull result, threw, stalled, or exhausted illegal-offset retries, the only thing the outcome depends on is "dead letters on this queue were not fully read". `scanIncomplete() = failedQueueCount > 0 || truncated` is exactly that coverage signal, and `classifyOutcome` consumes only that. - **The all-failed 502 rule generalizes cleanly.** "Every queue abandoned" and "every queue unreadable" have the same consequence — nothing was read — and an empty-but-SUCCESS report is wrong for both. Extending the existing `failedQueueCount == queues.size()` check to abandonment keeps one rule: the scan fails iff it read nothing and at least one queue was not cleanly exhausted (`topicMissing` stays the explicit carve-out for a group with no DLQ topic at all). - **No current consumer could act on the split differently.** `classifyOutcome` and the audit line branch only on incompleteness; the read paths surface `failedQueueCount` as a single opaque counter. The log lines already record which branch fired (`did not advance offset N` / `returned OFFSET_ILLEGAL N times consecutively`), so if a future caller needs to distinguish "read failed" from "abandoned after repeated corrections", the separation can be added when there is a consumer for it. That said, the separation is a small mechanical change (`abandonedQueueCount` alongside the counter, folded into `scanIncomplete()`), and I'm happy to do it if you prefer the two facts to stay distinguishable in the response VO. ### 4. The read-path behaviour change Confirmed — intended, and now covered by a test pushed in 1345c5d0 (`listMessagesFailsWhenTheOnlyDlqQueueStallsTest`). For a **single-queue** DLQ topic (the common case for small groups) whose only queue stalls, `failedQueueCount` now reaches `queues.size()` and the list/detail/export paths get the 502 scan failure where they previously degraded to an empty result. I consider that correct for the same reason as the resend path: an empty list on the DLQ page reads as "no dead letters in the window" — indistinguishable from a healthy empty DLQ — while dead letters exist, and the DLQ list is the surface users rescan and resend from. It also mirrors the asymmetry you noted: the read paths already 502 when every queue *throws*; this extends that same rule from "every queue failed to read" to "every queue failed to complete". The `topicMissing` carve-out is untouched, so a group that never dead-lettered a message still gets the empty-list degradation. The multi-queue mixed case is unchanged in shape: partial results, with `failedQueueCount` surfaced by the export paths. (The list path's `PageResult` doesn't carry the counter — that's a pre-existing surface limitation; happy to extend it in a follow-up if you want the incompleteness visible there too, but it felt out of scope for this fix.) ### Test evidence ``` $ mvn test -Dtest=RocketMQDLQProviderTest Tests run: 42, Failures: 0, Errors: 0, Skipped: 0 # 41 before; the new list-path test is the 42nd # mutation check: reverting just the two failedQueueCount++ increments Tests run: 42, Failures: 4 # the two resend tests, the rewritten one, and the new list-path test all fail ``` -- 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]
