You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "zhaohai666 (via GitHub)" <gi...@apache.org> on 2026/08/26 02:59:57 UTC
[PR] [Studio] fix: resolve code-quality and resource-leak findings [rocketmq-dashboard]
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Studio] fix: resolve code-quality and resource-leak findings [rocketmq-dashboard]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on code in PR #2619:
URL: https://github.com/apache/rocketmq-dashboard/pull/2619#discussion_r3859451615
##########
server/src/main/java/org/apache/rocketmq/studio/instance/dlq/DLQController.java:
##########
@@ -95,22 +97,25 @@ public Result<PageResult<DLQMessageVO>> listDLQMessages(@PathVariable String gro
}
@GetMapping("/export-excel")
- public ResponseEntity<byte[]> exportDLQExcel(@RequestParam String instanceId,
- @RequestParam String groupName,
- @RequestParam(required = false) Long startTime,
- @RequestParam(required = false) Long endTime,
- @Size(max = MAX_SELECTED_MESSAGES,
- message = "At most 100 msgIds are allowed per export")
- @RequestParam(required = false) List<String> msgIds) {
- DLQExcelExportResultVO result = dlqService.exportExcel(instanceId, groupName, startTime, endTime, msgIds);
- return ResponseEntity.ok()
- .header(HttpHeaders.CONTENT_DISPOSITION,
- attachmentDisposition("dlq-" + sanitizeForFilename(groupName) + ".xlsx").toString())
- .header(HEADER_EXPORT_TRUNCATED, String.valueOf(result.isTruncated()))
- .header(HEADER_EXPORT_FAILED_QUEUES, String.valueOf(result.getFailedQueueCount()))
- .header(HEADER_EXPORT_LIMIT, String.valueOf(result.getLimit()))
- .contentType(MediaType.parseMediaType(EXCEL_MEDIA_TYPE))
- .body(result.getData());
+ public ResponseEntity<Void> exportDLQExcel(@RequestParam String instanceId,
+ @RequestParam String groupName,
+ @RequestParam(required = false) Long startTime,
+ @RequestParam(required = false) Long endTime,
+ @Size(max = MAX_SELECTED_MESSAGES,
+ message = "At most 100 msgIds are allowed per export")
+ @RequestParam(required = false) List<String> msgIds,
+ HttpServletResponse response) throws IOException {
+ // Stream the workbook straight to the response body so it is never buffered in the heap.
+ response.setContentType(EXCEL_MEDIA_TYPE);
+ response.setHeader(HttpHeaders.CONTENT_DISPOSITION,
+ attachmentDisposition("dlq-" + sanitizeForFilename(groupName) + ".xlsx").toString());
Review Comment:
**[Warning]** Response headers set after streaming write.\n\nThe metadata headers (`X-DLQ-Export-Truncated`, `X-DLQ-Export-FailedQueues`, `X-DLQ-Export-Limit`) are set **after** `dlqService.exportExcel()` writes to `response.getOutputStream()`. In a real servlet container, if the Excel data exceeds the response buffer (typically 8 KB for Tomcat), the buffer auto-flushes during the write and the response becomes committed — subsequent `setHeader()` calls are silently ignored.\n\nFor large exports, the client would lose the completeness metadata headers, which defeats their purpose (the client uses them to detect truncation and failed queues).\n\n**Suggested fix:** Set all headers **before** the streaming write. Since the metadata is only available after the export completes, consider a two-phase approach:\n```java\n// Phase 1: write to a temp buffer / piped stream to collect metadata\n// Phase 2: set headers from metadata\n// Phase 3: pipe buffer to response output\n```\nOr restr
ucture `exportExcel` to return metadata before writing the body.
##########
server/src/main/java/org/apache/rocketmq/studio/provider/apache/RocketMQDLQProvider.java:
##########
@@ -335,23 +334,24 @@ public DLQExcelExportResultVO exportExcel(String instanceId, String groupName, L
DeadLetterScanResult scanResult = collectDeadLetters(instanceId, dlqTopic, begin, end, RESEND_HARD_CAP);
Set<String> selected = msgIds == null ? Collections.emptySet()
: new java.util.HashSet<>(msgIds);
- List<DLQMessageVO> messages = scanResult.messages().stream()
+ List<DLQMessageExcelRow> rows = scanResult.messages().stream()
.filter(message -> selected.isEmpty() || selected.contains(message.getMsgId()))
.map(this::toExportVO)
+ .map(DLQMessageExcelRow::from)
.toList();
- byte[] data;
try {
- ByteArrayOutputStream output = new ByteArrayOutputStream();
- com.alibaba.excel.EasyExcel.write(output, DLQMessageExcelRow.class)
+ // Stream straight to the caller's OutputStream so the whole workbook is never buffered
+ // in the heap (the export is capped at RESEND_HARD_CAP messages, but large snapshots can
+ // still be several MB).
+ com.alibaba.excel.EasyExcel.write(out, DLQMessageExcelRow.class)
.sheet("DLQ")
- .doWrite(messages.stream().map(DLQMessageExcelRow::from).toList());
- data = output.toByteArray();
+ .doWrite(rows);
+ out.flush();
} catch (Exception e) {
log.warn("Failed to build Excel export for group {}: {}", groupName, e.getMessage());
Review Comment:
**[Info]** Redundant flush. `out.flush()` is called here (line 351) and again in the controller (`response.getOutputStream().flush()`). The double flush is harmless but unnecessary — one flush at the controller level is sufficient.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Studio] fix: resolve code-quality and resource-leak findings [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins commented on PR #2619:
URL: https://github.com/apache/rocketmq-dashboard/pull/2619#issuecomment-5537075620
Closing after review: the streaming DLQ Excel export sets response headers after the response body has already started, so the headers are inevitably lost — the fix direction does not work as implemented. The 09-01 update did not address this. If you'd like to rework it (collect/buffer before committing headers, or stream with pre-set headers), feel free to reopen or submit a new PR. Thanks!
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Studio] fix: resolve code-quality and resource-leak findings [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins closed pull request #2619: [Studio] fix: resolve code-quality and resource-leak findings
URL: https://github.com/apache/rocketmq-dashboard/pull/2619
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org