zhaohai666 opened a new pull request, #2619:
URL: https://github.com/apache/rocketmq-dashboard/pull/2619
# PR: Studio code-quality and resource-leak cleanups
- **From:** `zhaohai666:feature/studio-optimization-cleanup`
- **Into:** `apache:rocketmq-studio`
- **Branch base:** `14da4b95` (`fix(studio): localize settings management
tabs (#2565)`)
- **Status:** backend tests green (47/47), `web` `tsc -b` clean
## Summary
A focused cleanup that resolves three concrete findings from the
`rocketmq-studio` code
review (the branch stood at `7443227f` when reviewed; this PR is rebased
onto the current
tip `14da4b95`). The changes are low-risk, self-contained, and covered by
tests.
| ID | Area | Change |
|----|------|--------|
| F2 | Frontend types | Remove duplicate `ResetConsumerOffsetRequest`
interface |
| B6 | Backend resources | Add `@PreDestroy` to `TencentClientFactory` |
| N1 | Backend memory | Stream DLQ Excel export to the response
`OutputStream` |
## Changes
### F2 — Duplicate `ResetConsumerOffsetRequest` interface
`web/src/api/metadata.ts` declared `ResetConsumerOffsetRequest` **twice**
(line 148 without
`instanceId`, line 310 with `instanceId?`). Because both are `interface`
declarations,
TypeScript merges them into the superset `{ name; timestamp; topic;
instanceId? }`, so the
project compiles — but the first definition is dead, confusing duplication
that invites
drift. Removed the `:148` definition; the surviving shape is a strict
superset of all current
callers (which route through `services/consumerService.ts`), so no call site
changes.
### B6 — `TencentClientFactory` resource leak on shutdown
`TencentClientFactory` caches one `TrocketClient` per `credential#region`
but had **no
`@PreDestroy`**, unlike `AliyunClientFactory` which already releases its
clients on context
shutdown. Added a `@PreDestroy close()` that clears the cache so the cached
clients (and their
underlying connection pools) are released instead of being retained for the
whole process
lifetime.
> Note: the Tencent SDK `AbstractClient` exposes no public
`shutdown()`/`close()` method, so
> the factory clears the cache to drop references and let GC reclaim the
clients. This mirrors
> the intent of the existing Aliyun `@PreDestroy` without calling a
non-existent API.
### N1 — DLQ Excel export heap buffering
`RocketMQDLQProvider.exportExcel` built the entire `.xlsx` workbook into a
`ByteArrayOutputStream` and returned it as a `byte[]`, which the controller
then copied into a
`ResponseEntity<byte[]>`. For a large export (capped at `RESEND_HARD_CAP =
5000` messages, but
still potentially several MB) this doubles the peak heap usage.
The export now streams directly to the HTTP response `OutputStream`:
- `DLQExcelExportResultVO` no longer carries the `byte[] data` field — it
holds only the
scan-completeness metadata (`truncated`, `failedQueueCount`, `limit`).
- `exportExcel(...)` gained an `OutputStream out` parameter; the provider
writes the sheet to
it via `EasyExcel.write(out, ...).doWrite(rows)` and flushes.
- `DLQController.exportDLQExcel` sets the content-type/disposition/metadata
headers on the
`HttpServletResponse` and streams the workbook to
`response.getOutputStream()`; it returns
`ResponseEntity<Void>`.
- `DLQControllerTest` updated to the new method arity; the two
`export-excel` tests now assert
the response headers (the bytes are streamed, so the mocked service no
longer returns them).
## Verification
- Backend (`mvn -o test`, JDK 21, local repo): `DLQControllerTest` 23,
`RocketMQDLQProviderTest` 20, `TencentClientFactoryTest` 4 — **47/47
pass**.
- Frontend: `npx tsc -b` clean.
## Out of scope (recommended as follow-ups)
The review surfaced additional findings that are intentionally **not** in
this PR:
- **Frontend de-duplication (F3–F7, F10, F11, A1)** —
`formatDateTime`/`getErrorMessage`/
sort helpers, `Table` column `useMemo`, render-time `setState`, error
double-toast, and the
`api/`↔`services/` double-exit. These are already implemented in the
unmerged branches
`feature/studio-frontend-optimizations` / `feature/studio-web-fixes` and
should be merged
directly rather than re-done here.
- **B4** — DLQ list still calls `examineTopicStats` per group (pagination
caps the page, but
it is still a per-group RPC). Suggested fix: compute the message count
from `Min/MaxOffset`
and lazy-load full stats on the detail view.
- **N2** — `RocketMQMessageProviderTest.queryByTopic*` expects
`MAX_PULLS_PER_QUEUE = 1000`
while the shipped constant is `32`; these two tests are red on trunk.
Needs a product
decision on the intended tail-budget size before adjusting the constant or
the assertions.
- **N3** — `/export-excel` and `/export` both scan dead letters; the scan
logic can be shared.
- **N4** — the newly added queue-browsing / direct-consume / DLQ-export
pages should be
re-checked against the F6/F7 anti-patterns.
--
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]
