Copilot commented on code in PR #10448:
URL: https://github.com/apache/rocketmq/pull/10448#discussion_r3377286763
##########
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:
The boolean parameter is named `removeOffset` here, but the protocol/body
field and broker-side logic use `cleanOffset`
(`DeleteSubscriptionGroupListRequestBody#cleanOffset` and
`requestBody.isCleanOffset()`). Renaming this parameter to `cleanOffset` (or
aligning names across client/body/broker) would reduce confusion and make it
clearer that this flag maps directly to the request body.
##########
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:
Same issue as the batch topic delete API: `assert response != null;` can be
compiled out and lead to `NullPointerException`. Replace the assertion with an
explicit null check and throw a well-formed exception so callers get a
deterministic failure mode.
##########
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:
The `deleteTopic(...)` method was changed from `synchronized` to
non-synchronized in this diff, while it still performs multi-step mutations
(topic config, offsets, queue mapping, message store deletions). This can
introduce races if multiple admin requests delete/modify topics concurrently
(including concurrent calls to the new batch API). Consider restoring
synchronization (or using a shared lock around both single and batch delete
paths) to keep broker state changes atomic relative to other admin operations.
##########
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:
The log message labels the placeholder as `topic`, but logs the full
`TopicConfig` object (`old`). This is misleading for operators and makes log
search harder. Consider logging the topic name/string (e.g., `topic` or
`old.getTopicName()`) and, if needed, a separate structured field for the full
config.
##########
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:
Using `assert response != null;` is fragile because assertions are typically
disabled in production JVMs; if `invokeSync` returns null, the subsequent
`response.getCode()` will throw a `NullPointerException` rather than a
controlled `MQClientException`. Prefer an explicit runtime check that throws an
`MQClientException` (or `RemotingException`) with a helpful remark when
`response` is null.
--
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]
