RockteMQ-AI commented on PR #10448: URL: https://github.com/apache/rocketmq/pull/10448#issuecomment-4790420858
## Review by github-manager-bot (Re-review after new commit `4f0ab43`) ### Summary Adds batch deletion APIs (`DELETE_TOPIC_IN_BROKER_LIST` / `DELETE_SUBSCRIPTIONGROUP_LIST`) to reduce N RPCs + N `persist()` calls to 1 RPC + 1 persist. The new commit restructures the diff but retains the separate batch cleanup path. ### Findings - **[Warning]** `AdminBrokerProcessor.java` — **Separate cleanup path vs. @fuyou001 suggestion.** The batch `deleteTopicList` maintains its own cleanup sequence rather than iterating over the existing `deleteTopic()`. This is intentional per the PR description (1 persist + 1 `deleteTopics()` call), and it does achieve the stated performance goal. However, any future fix to `deleteTopic()` (e.g., additional cleanup steps) must be manually mirrored here. Consider adding a comment at both methods cross-referencing each other, or extracting the common cleanup steps into a shared helper that both paths can call. - **[Warning]** `AdminBrokerProcessor.java:deleteTopicList` — **Partial failure semantics.** If `messageStore.deleteTopics()` succeeds but `deleteTopicConfigList()` throws, the physical data is gone but config metadata remains. On retry, `collectPopRetryTopics` can still discover derived topics via `selectTopicConfig`, which is good. But the caller gets `SYSTEM_ERROR` without knowing what was partially cleaned. Consider logging the successfully cleaned topics before the exception, or returning a partial-success response. - **[Info]** `MQClientAPIImpl.java:deleteTopicInBrokerList` — The `switch` with empty `case SUCCESS: return;` followed by `default: break;` then `throw` is functionally correct and consistent with the existing `deleteTopicInBroker` pattern. No action needed. - **[Info]** `DefaultAuthorizationContextBuilder.java` — Good handling of body-driven auth contexts. The comment explaining why the annotation-based path doesn't work for batch APIs is helpful for future maintainers. - **[Info]** `RequestCode.java` — New codes 5002/5003 are in the 5xxx range (timer/engine area). Not a bug, but worth confirming these don't collide with any in-flight PRs using the 5xxx range. - **[Info]** Tests — Good coverage: empty input, system topic rejection, dedup verification (single persist call), and batch subscription group deletion. The `testDeleteTopicListBatchPersist` test correctly verifies that `deleteTopicConfig` (singular) is NOT called on the batch path. ### Regarding @fuyou001 RateLimiter Suggestion The current approach prioritizes batch efficiency (1 persist + 1 store call). A `RateLimiter` wrapper around individual `deleteTopic()` calls would trade some throughput for safer burst control. This is a design decision for the maintainers — both approaches are valid. If the RateLimiter approach is adopted, the batch API can internally iterate with rate limiting while still saving the network round-trips. ### Assessment Well-structured addition that follows the existing `*_LIST` convention. Authorization, dedup, validation, and test coverage are all solid. The two warnings above (cleanup path divergence and partial failure) are worth addressing but not blockers. --- *Automated review by github-manager-bot* -- 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]
