wang-jiahua commented on code in PR #10448:
URL: https://github.com/apache/rocketmq/pull/10448#discussion_r3377345041
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -758,7 +766,7 @@ private synchronized RemotingCommand
updateAndCreateStaticTopic(ChannelHandlerCo
return response;
}
- private synchronized RemotingCommand deleteTopic(ChannelHandlerContext ctx,
+ private RemotingCommand deleteTopic(ChannelHandlerContext ctx,
RemotingCommand request) throws RemotingCommandException {
Review Comment:
Done in b3e851de2. Adopted the second alternative you suggested: introduced
a `private final Object adminTopicLock = new Object();` shared by both
`deleteTopic(...)` and `deleteTopicList(...)`. The processor-level
`synchronized` is gone, so unrelated admin operations (e.g.
`updateAndCreateSubscriptionGroup`, getters) are no longer blocked, but topic
delete paths remain mutually atomic. Concurrency between `deleteTopic` and
`updateAndCreateTopic*` is a pre-existing trade-off and is intentionally left
out of this PR's scope.
##########
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##########
@@ -2197,6 +2199,25 @@ public void deleteTopicInBroker(final String addr, final
String topic, final lon
throw new MQClientException(response.getCode(), response.getRemark());
}
+ public void deleteTopicInBrokerList(final String addr, final List<String>
topicList, final long timeoutMillis)
+ throws RemotingException, InterruptedException, MQClientException {
+ DeleteTopicListRequestBody requestBody = new
DeleteTopicListRequestBody(topicList);
+ RemotingCommand request =
RemotingCommand.createRequestCommand(RequestCode.DELETE_TOPIC_IN_BROKER_LIST,
null);
+ request.setBody(requestBody.encode());
+ RemotingCommand response =
this.remotingClient.invokeSync(MixAll.brokerVIPChannel(this.clientConfig.isVipChannelEnabled(),
addr),
+ request, timeoutMillis);
+ assert response != null;
Review Comment:
Done in b3e851de2. Replaced `assert response != null;` in
`deleteTopicInBrokerList` with an explicit null check that throws
`MQClientException(SYSTEM_ERROR, ...)`, so callers get a deterministic failure
even with assertions disabled.
##########
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##########
@@ -2257,6 +2278,27 @@ public void deleteSubscriptionGroup(final String addr,
final String groupName, f
throw new MQClientException(response.getCode(), response.getRemark());
}
+ public void deleteSubscriptionGroupList(final String addr, final
List<String> groupNameList, final boolean removeOffset,
+ final long timeoutMillis)
+ throws RemotingException, InterruptedException, MQClientException {
+ DeleteSubscriptionGroupListRequestBody requestBody = new
DeleteSubscriptionGroupListRequestBody(groupNameList, removeOffset);
+ RemotingCommand request =
RemotingCommand.createRequestCommand(RequestCode.DELETE_SUBSCRIPTIONGROUP_LIST,
null);
+ request.setBody(requestBody.encode());
+
+ RemotingCommand response =
this.remotingClient.invokeSync(MixAll.brokerVIPChannel(this.clientConfig.isVipChannelEnabled(),
addr),
+ request, timeoutMillis);
+ assert response != null;
Review Comment:
Done in b3e851de2. Same fix applied to `deleteSubscriptionGroupList`:
explicit null check + `MQClientException(SYSTEM_ERROR, ...)` instead of
`assert`.
##########
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##########
@@ -2257,6 +2278,27 @@ public void deleteSubscriptionGroup(final String addr,
final String groupName, f
throw new MQClientException(response.getCode(), response.getRemark());
}
+ public void deleteSubscriptionGroupList(final String addr, final
List<String> groupNameList, final boolean removeOffset,
Review Comment:
Done in b3e851de2. Renamed the new client API parameter from `removeOffset`
to `cleanOffset` so it now matches
`DeleteSubscriptionGroupListRequestBody#cleanOffset` and the broker-side
`requestBody.isCleanOffset()`. The legacy single-item
`deleteSubscriptionGroup(..., boolean removeOffset, ...)` is left untouched to
avoid breaking existing callers.
##########
broker/src/main/java/org/apache/rocketmq/broker/topic/TopicConfigManager.java:
##########
@@ -626,6 +626,26 @@ public void deleteTopicConfig(final String topic) {
}
}
+ public void deleteTopicConfigList(final List<String> topics) {
+ if (topics == null || topics.isEmpty()) {
+ return;
+ }
+ boolean changed = false;
+ for (String topic : topics) {
+ TopicConfig old = removeTopicConfig(topic);
+ if (old != null) {
+ log.info("delete topic config OK, topic: {}", old);
Review Comment:
Done in b3e851de2. Updated the log message in the new
`deleteTopicConfigList` (and the analogous one added in
`SubscriptionGroupManager#deleteSubscriptionGroupConfigList`) to log the topic
/ group name instead of the full config object, so the placeholder name now
matches the value. The original `deleteTopicConfig` /
`deleteSubscriptionGroupConfig` log lines are pre-existing and outside this
PR's scope.
--
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]
