lizhimins commented on PR #4677:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4677#issuecomment-5761165214
Thanks — this is a substantial PR and several parts of it are solid.
**What we verified as correct.** The two performance complaints are real:
`RocketMQDLQProvider.listDLQGroups` collects the current page's stats through a
serial stream, and `dlq.tsx` feeds `search` straight into the load effect's
dependency list with `onChange` writing it on every keystroke. Page order is
preserved and asserted (`containsExactly` over the group names), all futures
are joined inside the `runtimeAdminClientResolver.execute` lambda so no client
is released early, the `InterruptedException` branch restores the interrupt
flag, the debounce does not slow the first paint, and the thread pool is named,
daemonized and shut down in `@PreDestroy`. The three new Java tests and two new
`it()` blocks are genuine and mutation-sensitive — reverting the parallel path
makes the `CountDownLatch` case fail through the `Error` channel that
`buildDLQGroup`'s `catch (Exception)` does not swallow, and the test counts in
your description match the diff exactly. No tool-catalog change is
needed either: `group.yaml` declares `status` and `statsAvailable` as plain
strings/booleans with no enum, and `"UNAVAILABLE"` is already emitted on the
base branch.
**Why we are closing.** Two things, both structural.
1. **It collides with #4577, which we are taking forward.** #4577 was opened
about 26 hours before this PR and adds `setSearch('')` to the instance-scoped
reset block in `dlq.tsx`. Once your `searchInput` / `search` split lands, that
line clears only the request value: the input keeps displaying the old term,
and your debounce effect sees `searchInput.trim() !== search` and writes the
old term back into `search` 300 ms later. The result is that a newly selected
instance gets filtered by the previous instance's group name — precisely the
bug #4577 exists to fix, and its own assertion would fail. `git merge-tree`
reports this as clean because the hunks do not overlap, so nothing in CI would
have caught it. #4577 is first in the queue on DLQ search semantics and is the
smaller, safer change, so that is the one landing.
2. **The shared executor's blast radius is not accounted for.**
`statsExecutor` is a field of a `@Service @Primary` singleton,
`Executors.newFixedThreadPool` gives it an unbounded `LinkedBlockingQueue`, and
it would serve *both* the paged UI path and the unpaged
`listDLQGroups(instanceId)` — which passes `Integer.MAX_VALUE` as the page size
and whose only caller is the background collector
`ApacheRocketMqDlqMetricsCollector.collect()`. One collection sweep over every
`%DLQ%` topic therefore queues potentially thousands of tasks on the same 8
threads the UI depends on. Because `future.get(5, SECONDS)` is called in a
serial loop, the 5-second budget includes queue wait, so a healthy page can
come back with rows marked `UNAVAILABLE`; `ApacheRocketMqDlqMetricsCollector`
maps that to `MetricAvailability.UNAVAILABLE`, and `AlertStateMachine`
documents UNAVAILABLE as "the sole opt-in path that turns a failed probe into a
firing alert" — i.e. pool contention can raise false DLQ alerts
. None of the three new backend tests feeds more than a handful of topics, so
saturation is untested.
One correction to the description, stated plainly because it is checkable:
"fallback rows preserve `groupName` and `dlqTopic`, so failed stats calls no
longer produce blank table rows" is already true on the base branch.
`buildDLQGroup` catches `Exception`, sets `statsAvailable = false`, and returns
a row carrying `groupName`, `dlqTopic` and `status("UNAVAILABLE")`; your
`unavailableGroup()` is behaviourally identical to that existing branch. The
genuinely new backend behaviour is that an `Error` or a timeout now degrades
one row instead of failing the whole page.
**What would make a future attempt welcome.** Once #4577 has landed, a
backend-only PR that parallelizes just the paged UI path — with a bounded queue
and an explicit rejection policy, or a separate executor for the collector so
the two cannot starve each other — plus a test that actually saturates the
pool, would be a good change and we would review it on its merits. The debounce
is worth sending separately as a frontend PR, rebased on whatever search-reset
shape #4577 leaves behind.
--
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]
