[ISSUE #10446] Support batch deletion of topics and subscription groups in broker

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #10446

Brief Description

Adds two new batch admin APIs to AdminBrokerProcessor whose names follow the existing *_LIST convention (e.g. UPDATE_AND_CREATE_TOPIC_LIST, UPDATE_AND_CREATE_SUBSCRIPTIONGROUP_LIST):

• DELETE_TOPIC_IN_BROKER_LIST
• DELETE_SUBSCRIPTIONGROUP_LIST

Bulk metadata cleanup (e.g. tenant decommissioning, retention purge, cluster migration) currently requires N round-trips to delete N items and triggers N persist() calls on the broker config files. These new APIs allow doing the same in 1 RPC + 1 persist + 1 messageStore.deleteTopics(Set) call.

Note: removing the synchronized modifier from AdminBrokerProcessor#deleteTopic is out of scope here and is already being discussed in #9997 / #9998. This PR keeps synchronized as-is for both the existing deleteTopic and the new deleteTopicList, so the lock semantics match the rest of the existing updateAndCreate* family.

Changes

• RequestCode: add DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST.
• New request bodies:
  • DeleteTopicListRequestBody { List<String> topicList }
  • DeleteSubscriptionGroupListRequestBody { List<String> groupNameList; boolean cleanOffset }
• TopicConfigManager#deleteTopicConfigList(List<String>) and SubscriptionGroupManager#deleteSubscriptionGroupConfigList(List<String>): aggregate persist() once per batch.
• AdminBrokerProcessor:
  • Extract collectPopRetryTopics(...) shared helper used by both single and batch paths.
  • Add deleteTopicList(...) and deleteSubscriptionGroupList(...) (both synchronized, consistent with existing single APIs) with input dedup (preserving order), system-topic / blank validation, one-shot persist, and one-shot messageStore.deleteTopics(...) call.
• MQClientAPIImpl: add deleteTopicInBrokerList(...) and deleteSubscriptionGroupList(...) client helpers; both use an explicit null check (instead of assert) so callers get a deterministic MQClientException(SYSTEM_ERROR, ...) when invokeSync returns null.

Compatibility

• Single-item DELETE_TOPIC_IN_BROKER / DELETE_SUBSCRIPTIONGROUP request codes and methods are unchanged; existing clients are not affected.
• New *_LIST request codes are additive; older brokers will fall through to getUnknownCmdResponse, so older clients/brokers continue to work as today.

How Did You Test This Change?

New unit tests added in AdminBrokerProcessorTest:

• testDeleteTopicListInBroker — covers empty input, system-topic rejection, and the success path.
• testDeleteTopicListBatchPersist — verifies that:
  • the manager-level deleteTopicConfigList is invoked once (so persist() runs once for the whole batch),
  • the singular deleteTopicConfig is not called on the batch path,
  • messageStore.deleteTopics(...) is invoked once with the batched set,
  • duplicate inputs are deduplicated.
• testDeleteSubscriptionGroupList — covers empty input and the success path with cleanOffset=true.

Local verification:

mvn -pl remoting,client,broker -Dspotbugs.skip=true validate
# 0 Checkstyle violations across all three modules

mvn -pl broker -Dcheckstyle.skip -Dspotbugs.skip=true -Djacoco.skip=true \
  -Dtest='AdminBrokerProcessorTest#testDeleteTopic+testDeleteTopicListInBroker+testDeleteTopicListBatchPersist+testDeleteSubscriptionGroup+testDeleteSubscriptionGroupList+testDeleteWithPopRetryTopic' \
  test
# Tests run: 6, Failures: 0, Errors: 0, Skipped: 0

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds broker/admin support for batch deletion of topics and subscription groups via new remoting request codes and request bodies, plus client APIs and broker-side handling.

Changes:

• Introduce new request bodies and request codes for batch-delete operations.
• Implement broker processor logic to delete topic/group lists (including dedup + optional cleanup behaviors).
• Add persistence helpers for batch config deletion and tests covering the new admin paths.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/DeleteTopicListRequestBody.java	Adds request body for batch topic deletion.
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/DeleteSubscriptionGroupListRequestBody.java	Adds request body for batch subscription-group deletion (with cleanOffset flag).
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RequestCode.java	Adds new request codes for batch delete operations.
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java	Adds client APIs to invoke the new batch delete broker requests.
broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java	Adds unit tests for batch deletion request handling and persistence behavior.
broker/src/main/java/org/apache/rocketmq/broker/topic/TopicConfigManager.java	Adds batch topic-config deletion with single persist/update.
broker/src/main/java/org/apache/rocketmq/broker/subscription/SubscriptionGroupManager.java	Adds batch subscription-group deletion with single persist/update.
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java	Routes new request codes and implements batch delete handlers; refactors pop retry topic collection.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[oss-sentinel-ai]

Review by github-manager-bot

Summary

This PR addresses two related performance issues in AdminBrokerProcessor:

1. Removes synchronized from deleteTopic since topicConfigTable is already a ConcurrentHashMap
2. Adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST) to reduce network round-trips

Findings

• [Good] AdminBrokerProcessor.java:769 — Removed synchronized from deleteTopic. The underlying topicConfigTable is a ConcurrentHashMap, so per-key remove is atomic. This eliminates unnecessary serialization of concurrent admin delete requests.
• [Good] AdminBrokerProcessor.java — Added deleteTopicList() and deleteSubscriptionGroupList() handlers following the existing *_LIST convention (consistent with UPDATE_AND_CREATE_TOPIC_LIST).
• [Good] DeleteTopicListRequestBody.java / DeleteSubscriptionGroupListRequestBody.java — Clean request body classes extending RemotingSerializable, following existing patterns.
• [Good] MQClientAPIImpl.java:2202-2220 / 2281-2301 — Client-side batch deletion methods properly implemented with timeout handling.
• [Good] RequestCode.java — Added DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST request codes.
• [Good] TopicConfigManager.java / SubscriptionGroupManager.java — Added deleteTopicList() and deleteSubscriptionGroupList() batch methods.
• [Good] AdminBrokerProcessorTest.java — Comprehensive test coverage including:
  • Empty list validation (returns INVALID_PARAMETER)
  • System topic rejection in batch
  • Happy path with multiple topics
  • Duplicate handling in batch
  • Batch persist verification

Analysis

The implementation is well-structured:

1. Backward compatibility: Existing single-item deletion APIs remain unchanged
2. Consistent patterns: Follows the *_LIST convention established by CreateTopicListRequestBody
3. Proper validation: Empty lists and system topics are rejected
4. Test coverage: Edge cases are well covered

Suggestions

1. Consider adding a maximum batch size limit to prevent excessive memory usage or long-running operations.
2. The batch persist test verifies deduplication — good attention to edge cases.

Verdict

✅ Approved — Well-designed performance optimization with proper validation and comprehensive test coverage.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 73.18436% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.21%. Comparing base (10d498c) to head (1038b33).

Files with missing lines	Patch %	Lines
...ocketmq/broker/processor/AdminBrokerProcessor.java	83.83%	6 Missing and 10 partials ⚠️
...l/body/DeleteSubscriptionGroupListRequestBody.java	56.25%	7 Missing ⚠️
.../broker/subscription/SubscriptionGroupManager.java	0.00%	6 Missing ⚠️
...ache/rocketmq/broker/topic/TopicConfigManager.java	0.00%	6 Missing ⚠️
.../java/org/apache/rocketmq/common/BrokerConfig.java	25.00%	6 Missing ⚠️
...on/builder/DefaultAuthorizationContextBuilder.java	77.77%	1 Missing and 3 partials ⚠️
...ting/protocol/body/DeleteTopicListRequestBody.java	62.50%	3 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10448      +/-   ##
=============================================
- Coverage      48.26%   48.21%   -0.05%     
- Complexity     13427    13445      +18     
=============================================
  Files           1378     1380       +2     
  Lines         100822   100987     +165     
  Branches       13036    13069      +33     
=============================================
+ Hits           48658    48692      +34     
- Misses         46217    46318     +101     
- Partials        5947     5977      +30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Adds batch deletion APIs for topics (DELETE_TOPIC_IN_BROKER_LIST) and subscription groups (DELETE_SUBSCRIPTIONGROUP_LIST), with proper per-resource authorization, input validation, and atomic-all-or-nothing semantics.

Findings

• [Info] Authorization — Correctly emits one DELETE context per resource in the batch, preventing bypass when the body carries the target list instead of the header. This is a security-critical detail handled well.
• [Info] Error handling — Returns a structured error response with the list of failed items and the first exception. Good for debugging partial failures.
• [Info] Dedup — Uses LinkedHashSet to preserve insertion order while deduplicating. Reasonable choice.
• [Info] Refactoring — Extracts collectPopRetryTopics() from inline code in deleteTopic(). Good cleanup that avoids duplication.
• [Info] Cleanup order — Deletes client config and subscription first, then topic config last. Allows retry after partial failure.
• [Warning] AdminBrokerProcessor.deleteTopicInBrokerList() — The RemotingCommand response is created once at the top and reused. If deleteTopicInBroker internally sets response fields, ensure there's no state leakage between the success and error paths.
• [Warning] MQClientAPIImpl.deleteTopicInBrokerList() — The method creates the request body and sends it. Consider adding a timeout or batch size limit to prevent excessively large requests.

Suggestions

• Consider adding a max batch size constant (e.g., 100) to prevent abuse or accidental large requests.
• The DeleteTopicListRequestBody and DeleteSubscriptionGroupListRequestBody classes are clean. Consider adding @AllArgsConstructor for convenience if not already present.
• Cross-repo: If batch deletion is exposed in the dashboard or CLI, ensure those clients are updated to use the new APIs.

Overall: Well-structured feature with proper authorization and error handling. A few minor suggestions but nothing blocking.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Adds two new batch admin APIs — DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST — to AdminBrokerProcessor, following the existing *_LIST convention (e.g. UPDATE_AND_CREATE_TOPIC_LIST). Includes proper authorization context building for body-driven batch requests, new request body DTOs, and test coverage.

Findings

• [Info] Authorization handling for body-driven batch APIs is correctly implemented — the DefaultAuthorizationContextBuilder decodes the request body and emits one DELETE context per item, preventing the annotation-based path from producing an empty context list (which would bypass permission checks).
• [Info] Good test coverage in DefaultAuthorizationContextBuilderTest — verifies blank entries are filtered, correct resource types, and per-item DELETE action.
• [Warning] The batch deletion is not atomic — if one topic/group fails to delete (e.g. still in use), the remaining items in the list may still be deleted. Consider documenting this partial-failure behavior in the API response, or adding a failFast flag.
• [Info] LinkedHashSet for deduplication while preserving insertion order is a good choice for predictable behavior.
• [Info] New DeleteTopicListRequestBody and DeleteSubscriptionGroupListRequestBody follow the existing pattern of CreateTopicListRequestBody.

Suggestions

• Consider adding a response summary that reports per-item success/failure status (similar to how batch operations in other systems report partial results). This would help callers understand which items succeeded when partial failure occurs.
• Add a brief comment in AdminBrokerProcessor documenting the non-atomic semantics of batch deletion.

No blocking issues. Well-structured addition that follows existing conventions.

---

Automated review by github-manager-bot

[wang-jiahua]

Server Benchmark Results (2026-06-22)

Environment: 8C30G broker, Dragonwell JDK 21, 1KB msg, 128 queues, sync mode, 256 threads, 20s warmup + 30s collect

Producer-only (steady state, lines 3-5)

Test	TPS	Avg RT	GC	vs Baseline
Baseline	217k	1.18ms	354	—
#10448	216k	1.19ms	296	Parity (feature PR, no perf change) ✅

This is a feature PR (batch delete Topic/SubscriptionGroup), not a performance optimization. Benchmark confirms no regression.

[fuyou001]

For deleteTopicList, I suggest reusing the existing single-topic delete path instead of maintaining a separate cleanup sequence. The batch handler can iterate over the requested topics, call deleteTopic(...) for each one, and guard the loop with a RateLimiter such as RateLimiter.create(10.0) so it deletes at most 10 topics per second.

That keeps the batch API aligned with the current single-delete semantics and avoids a large batch request creating a sudden cleanup burst on the broker. It would also be good to add a regression test covering the throttled batch path and invalid-entry handling before the loop starts.

[RockteMQ-AI]

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

[RockteMQ-AI]

Review by github-manager-bot

Summary

Updated review for the latest push (4f0ab43). This PR adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST, DELETE_SUBSCRIPTIONGROUP_LIST) to AdminBrokerProcessor, following the existing *_LIST convention. Includes per-resource authorization context building, new request body DTOs, client helpers, and test coverage.

What Changed Since Last Review

• Single commit (4f0ab43) rebased/updated on 2026-06-24.
• A new reviewer suggestion from @fuyou001 proposes reusing the single-topic delete path with a RateLimiter instead of a separate batch cleanup sequence.

Findings

• [Info] Authorization — DefaultAuthorizationContextBuilder correctly decodes the request body and emits one DELETE context per resource. This prevents the annotation-based path from producing an empty context list (which would bypass permission checks). Well handled.
• [Info] Dedup + ordering — LinkedHashSet preserves insertion order while deduplicating inputs. Good choice for predictable behavior.
• [Info] Refactoring — collectPopRetryTopics(...) is extracted from inline code in deleteTopic(), now shared by both single and batch paths. Reduces duplication.
• [Info] Batch persist — TopicConfigManager.deleteTopicConfigList() and SubscriptionGroupManager.deleteSubscriptionGroupConfigList() aggregate persist() once per batch instead of N times. Significant efficiency gain for large batches.
• [Info] Error reporting — Returns structured error with the list of failed items and the first exception. Good for debugging partial failures.
• [Warning] No batch size limit — Neither deleteTopicList nor deleteSubscriptionGroupList caps the input list size. A caller could send thousands of items in a single RPC, causing a long-running synchronized block and a large messageStore.deleteTopics(...) call. Consider adding a max batch size constant (e.g., 100 or 200) and returning REQUEST_CODE_NOT_SUPPORTED or a validation error when exceeded.
• [Warning] Rate limiting concern (raised by @fuyou001) — The current batch path does a burst of all deletes in one shot. If the reviewer suggestion is adopted (iterate + RateLimiter), it would smooth out the cleanup burst. If not adopted, at minimum document why a burst is acceptable for the expected use cases (tenant decommissioning, retention purge).
• [Warning] synchronized scope — Both deleteTopicList and deleteSubscriptionGroupList are synchronized, consistent with existing single-item methods. However, the batch path holds the lock for the entire batch duration. For large batches this could block other admin operations. This is acknowledged as out-of-scope (#9997/#9998) but worth noting for reviewers.
• [Info] Client null-check — MQClientAPIImpl.deleteTopicInBrokerList() and deleteSubscriptionGroupList() use explicit null checks on the response (instead of assert), producing a deterministic MQClientException(SYSTEM_ERROR, ...). Good defensive coding.
• [Info] Backward compatibility — New *_LIST request codes are additive; older brokers will return REQUEST_CODE_NOT_SUPPORTED via getUnknownCmdResponse. No breaking changes.

Suggestions

1. Add a max batch size guard — e.g., if (topicList.size() > MAX_BATCH_SIZE) { return error response; }. This is a low-cost safety measure.
2. Address @fuyou001 feedback — Either adopt the RateLimiter approach or explain why the current burst behavior is acceptable for the target use cases.
3. Consider adding a timeout parameter — The client methods accept timeoutMillis but the broker-side processing has no internal timeout. For very large batches, the broker could log a warning if processing exceeds a threshold.

Overall: Solid implementation with proper authorization, error handling, and test coverage. The main concern is the lack of batch size limits and the reviewer suggestion about rate limiting. Nothing blocking, but worth addressing before merge.

---

Automated review by github-manager-bot

[wang-jiahua]

@fuyou001 Thanks for the suggestion!

For deleteTopicList, I suggest reusing the existing single-topic delete path instead of maintaining a separate cleanup sequence.

We ran a broker-side A/B experiment comparing both approaches with the same batch RPC (only the broker JAR differs):

• Variant A (your suggestion): deleteTopicConfigList() loops deleteTopicConfig() → each call triggers persist() (3 × fsync per item)
• Variant B (current): deleteTopicConfigList() removes all from memory, then calls persist() once (3 fsync total)

Server benchmark results (50 topics/groups, Producer+Consumer running at ~180k TPS):

Metric	Variant A (loop persist)	Variant B (batch persist)	Speedup
Topic batch delete	730ms	133ms	5.5x
Group batch delete	397ms	18ms	22x
Producer TPS impact	0%	0%	—
Producer P99 RT	5ms	5ms	—
GC delta	16	16	—

The key bottleneck is persist() → fsync. Each deleteTopicConfig() call serializes the full config to JSON, writes to disk, then fsyncs file + directory. With 50 items, that's 150 fsync calls in Variant A vs 3 in Variant B.

Both variants have zero impact on concurrent Producer/Consumer TPS and P99 RT because the delete operation completes in under 1 second either way. The difference is in the delete operation latency itself — important for admin tools that wait for the response.

Regarding code reuse concern: The batch path only diverges at the removeTopicConfig + single persist() level. All other cleanup logic (queue mapping, consumer offset, pop inflight counter, timer metrics, message store) is shared and unchanged.

Would appreciate your feedback!

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

Summary

Re-review after rebase. PR adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST, DELETE_SUBSCRIPTIONGROUP_LIST) following the existing *_LIST convention. The rebase preserves the previous design: body-driven batch requests, per-item authorization contexts, single-persist batching, and structured partial-failure responses.

Findings

• [Info] Authorization — Body-driven batch APIs correctly emit one DELETE context per resource in DefaultAuthorizationContextBuilder, preventing the annotation-based path from producing an empty context list. Verified for both topic and subscription group batch codes.
• [Info] Batch persist — TopicConfigManager#deleteTopicConfigList and SubscriptionGroupManager#deleteSubscriptionGroupConfigList aggregate into a single persist() call per batch, avoiding N writes. Good.
• [Info] Dedup — LinkedHashSet preserves insertion order while deduplicating input. Consistent with existing *_LIST patterns.
• [Info] Refactoring — collectPopRetryTopics() extraction is clean and shared between single and batch delete paths, reducing duplication.
• [Info] Client null-check — MQClientAPIImpl methods use explicit response == null check (instead of assert) and throw MQClientException(SYSTEM_ERROR, ...). Deterministic behavior.
• [Info] Request codes — 5002 / 5003 are additive; older brokers return getUnknownCmdResponse for unknown codes. No backward compatibility risk.
• [Warning] AdminBrokerProcessor#deleteTopicList — The RemotingCommand response is created once at the top. On the success path, response.setCode(ResponseCode.SUCCESS) and response.setRemark(null) are set explicitly. On the error path, a DeleteTopicListResponseBody is serialized into the response body. This is correct, but verify that no intermediate code path mutates response fields between the two branches (e.g., a logging interceptor that calls response.setRemark()).
• [Warning] No max batch size — Neither the server-side handler nor the client method enforces a maximum batch size. A caller could send thousands of topics in a single RPC, causing a long synchronized hold on the broker. Consider adding a server-side guard (e.g., reject if topicList.size() > 1000) to prevent accidental or malicious abuse.

Suggestions

• Consider adding a MAX_BATCH_SIZE constant (e.g., 1000) and rejecting oversized requests early in deleteTopicList() / deleteSubscriptionGroupList() with a clear error message.
• The DeleteSubscriptionGroupListRequestBody has a 2-arg constructor (groupNameList, cleanOffset) and a 1-arg constructor (groupNameList). The 1-arg constructor could delegate to the 2-arg one (this(groupNameList, false)) to reduce duplication.

Verdict

Rebase looks clean — previous findings still hold. No new issues introduced. The two warnings above are non-blocking suggestions for future hardening.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

Summary

Re-review after new commit (2026-06-26). The new commit adds rate limiting to deleteTopicList via a configurable batchDeleteTopicMaxRate in BrokerConfig, addressing the previous "no max batch size" warning.

Findings

• [Info] Rate limiting added — BrokerConfig#batchDeleteTopicMaxRate (default 10.0) + Guava RateLimiter in deleteTopicList(). This throttles per-topic deletion speed, partially addressing the previous batch-size concern. Good improvement.
• [Warning] Rate limiter only in deleteTopicList, not in deleteSubscriptionGroupList — The subscription group batch delete has no equivalent rate limiting. If the goal is to prevent long synchronized holds, both methods should be consistent. Consider adding the same pattern to deleteSubscriptionGroupList.
• [Warning] Rate limiter inside synchronized method — deleteTopicList() is synchronized. With rate limiting at 10/s, a batch of 1000 topics would hold the lock for ~100 seconds. This could block other admin operations. Consider:
  1. Moving rate-limited work outside the synchronized block (e.g., validate + prepare under lock, then rate-limited delete outside), or
  2. Adding a max batch size guard (e.g., reject if topicList.size() > 1000) in addition to rate limiting.
• [Info] LiteMetadataUtil.isLiteMessageType() and getLiteLifecycleManager().cleanByParentTopic() are called inside the rate-limited loop. Verify these are lightweight operations; if they involve RPC or heavy I/O, the rate limiter may not be sufficient to prevent long holds.
• [Info] Previous findings (authorization, batch persist, dedup, refactoring, client null-check, request codes) remain valid and unchanged.

Suggestions

1. Add the same rate limiting pattern to deleteSubscriptionGroupList for consistency.
2. Consider adding a MAX_BATCH_SIZE constant as a hard upper bound, with rate limiting as a secondary throttle.
3. Document batchDeleteTopicMaxRate in the config description (e.g., "Max topics deleted per second in batch delete API. 0 means no limit.").

Verdict

New rate limiting is a positive step. The two warnings above (inconsistency with subscription group path, synchronized + rate limiter interaction) are non-blocking but worth addressing before merge.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (Re-review #2)

Summary

Re-review after new commit a258b0e (2026-06-26). This PR adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST, DELETE_SUBSCRIPTIONGROUP_LIST) to AdminBrokerProcessor, along with auth context integration, rate limiting, and client helpers.

Findings

• [Critical] AdminBrokerProcessor.java — deleteTopicList() does not call messageStore.deleteTopics(topicsToClean). The single-item deleteTopic() calls this.brokerController.getMessageStore().deleteTopics(topicsToClean) to clean CommitLog/ConsumeQueue data, but the batch path only calls deleteTopicInBroker(topic) per topic (which deletes config + queue mapping, not message store data). This will orphan message store data on disk. The batch path must include the messageStore.deleteTopics() call before or after the per-topic loop.

• [Warning] TopicConfigManager.java:629 / SubscriptionGroupManager.java:399 — deleteTopicConfigList() and deleteSubscriptionGroupConfigList() are defined but never called anywhere in this PR. deleteTopicList() calls deleteTopicInBroker() → deleteTopicConfig() one-by-one. Either these batch methods should be used (to aggregate persist() as the PR description claims), or they should be removed to avoid dead code.

• [Warning] AdminBrokerProcessor.java — The PR description states "aggregate persist() once per batch" and "one-shot persist", but the current implementation calls deleteTopicInBroker(topic) per topic, which internally calls deleteTopicConfig(topic) — each of which likely triggers its own persist(). If batch persist is a goal, the deleteTopicConfigList() method should override the loop to persist once at the end, and deleteTopicList() should call it.

• [Info] DefaultAuthorizationContextBuilder.java:324-362 — Auth context integration looks correct. Body-driven batch requests properly emit one DELETE context per topic/group, with blank entries filtered. Good that the annotation-based path gap was identified and handled explicitly.

• [Info] AdminBrokerProcessor.java:815-820 — collectPopRetryTopics() refactoring is clean and correctly shared between single and batch paths. The new ArrayList<>(topicsToClean) snapshot before mutating the set in the batch path is a good defensive copy.

• [Info] BrokerConfig.java:382 — batchDeleteTopicMaxRate default of 10.0 permits/sec is conservative and sensible. The maxRate > 0 guard to disable the rate limiter is clean.

• [Info] RequestCode.java:314-315 — Codes 5002/5003 are sequential after SWITCH_TIMER_ENGINE = 5001. No conflicts detected.

Suggestions

1. Must fix: Add messageStore.deleteTopics() call in deleteTopicList(). Suggested placement — before the per-topic deleteTopicInBroker loop, matching the single-item flow:

// After collecting topicsToClean (including pop retry topics):
try {
    if (this.brokerController.getMessageStore() != null) {
        this.brokerController.getMessageStore().deleteTopics(topicsToClean);
    }
} catch (Throwable t) {
    return buildErrorResponse(ResponseCode.SYSTEM_ERROR, t.getMessage());
}
// Then the existing rate-limited deleteTopicInBroker loop

2. Should fix: Either wire deleteTopicConfigList() / deleteSubscriptionGroupConfigList() into the batch path (with single-persist optimization), or remove them.

3. Nit: deleteSubscriptionGroupList() reuses batchDeleteTopicMaxRate for group deletion rate limiting. Consider renaming to batchDeleteMaxRate for clarity, or adding a separate config for group deletion.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (Re-review #3)

Summary

Re-review after new commit 1038b335 (2026-06-26). This PR adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST, DELETE_SUBSCRIPTIONGROUP_LIST) to AdminBrokerProcessor, with auth context integration, rate limiting, and client helpers.

Changes Since Last Review

The latest commit appears to be a rebase/squash. The overall structure remains the same as the previous review.

Findings

• [Critical] AdminBrokerProcessor.java — deleteTopicList() does not call this.messageStore.deleteTopics(topicsToClean). The single-item deleteTopic() calls it after the per-topic loop to clean up actual message data in the store. Without this call, batch topic deletion removes broker-side metadata (topic config, queue mapping, etc.) but leaves orphaned message data on disk. This is the same issue flagged in the previous two reviews and remains unresolved.

  Suggested fix — add the store cleanup after the loop in deleteTopicList():

  for (String topic : topicsToClean) {
      if (rateLimiter != null) { rateLimiter.acquire(); }
      if (LiteMetadataUtil.isLiteMessageType(topic, brokerController)) {
          brokerController.getLiteLifecycleManager().cleanByParentTopic(topic);
      }
      deleteTopicInBroker(topic);
  }
  // ── add this ──
  this.messageStore.deleteTopics(topicsToClean);

• [Warning] AdminBrokerProcessor.java:deleteTopicList() — The method is synchronized, same as deleteTopic(). For a batch operation that may delete many topics with rate-limiting delays, holding the broker-wide lock for the entire duration could block other admin operations. Consider whether the critical section can be narrowed (e.g., validate first under the lock, then delete outside it).

• [Info] SubscriptionGroupManager.deleteSubscriptionGroupConfigList() and TopicConfigManager.deleteTopicConfigList() — These new batch methods simply loop over the single-item methods. They are not currently called by deleteTopicList() / deleteSubscriptionGroupList() (which call the single-item methods directly via deleteTopicInBroker() / deleteSubscriptionGroupInBroker()). If these batch methods are not used elsewhere, consider removing them to avoid dead code. If they are intended for future use, a brief Javadoc explaining the intended caller would help.

• [Info] RequestCode.java — New codes 5002 / 5003 are registered. No conflicts detected with existing codes. Good.

What Looks Good

• Clean extraction of collectPopRetryTopics() from the single-item path — reduces duplication.
• deleteSubscriptionGroupInBroker() correctly checks isLiteGroupType before deleting the group config (avoids auto-recreation when autoCreateSubscriptionGroup=true).
• Auth context builder correctly emits per-resource DELETE actions for the batch request codes.
• Rate limiting via Guava RateLimiter with configurable max rate is a good safety measure.
• Comprehensive test coverage: empty/blank/system-topic validation, dedup, rate limiting, pop-retry-topic collection.

---

Automated review by github-manager-bot