You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/01/04 02:29:50 UTC
[GitHub] [rocketmq-dashboard] zhangjidi2016 opened a new pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
zhangjidi2016 opened a new pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57
## What is the purpose of the change
#55
## Brief changelog
XX
## Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice, `it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR`.
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
- [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test` to make sure integration-test pass.
- [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling merged pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling merged pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777804960
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -295,6 +301,9 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
for (String brokerName : deleteSubGroupRequest.getBrokerNameList()) {
logger.info("addr={} groupName={}", clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr(), deleteSubGroupRequest.getGroupName());
mqAdminExt.deleteSubscriptionGroup(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr(), deleteSubGroupRequest.getGroupName(), true);
+ // delete %RETRY%+Group and %DLQ%+Group in broker and namesrv
+ deleteDlqOrRetryTopic(MixAll.RETRY_GROUP_TOPIC_PREFIX + deleteSubGroupRequest.getGroupName(), brokerName, clusterInfo);
+ deleteDlqOrRetryTopic(MixAll.DLQ_GROUP_TOPIC_PREFIX + deleteSubGroupRequest.getGroupName(), brokerName, clusterInfo);
Review comment:
deleteResources will be better than the existing method name.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#issuecomment-1006187071
@zhangjidi2016 would you like to confirm why coverage drop a bit of coverage? We could fix it in the next pr.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777805673
##########
File path: src/test/java/org/apache/rocketmq/dashboard/controller/ConsumerControllerTest.java
##########
@@ -182,6 +182,8 @@ public void testDelete() throws Exception {
final String url = "/consumer/deleteSubGroup.do";
{
doNothing().when(mqAdminExt).deleteSubscriptionGroup(any(), anyString());
+ doNothing().when(mqAdminExt).deleteTopicInBroker(any(), anyString());
Review comment:
Do we have a better operation than do-nothing when delete happend?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#issuecomment-1006197742
>
OK, code not covered by the previous PR(#59) has been covered in this PR(#63) unit test.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778494920
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -312,14 +320,16 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
return true;
}
- private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo) throws Exception {
+ private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo, boolean deleteInNsFlag) throws Exception {
mqAdminExt.deleteTopicInBroker(Sets.newHashSet(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr()), topic);
Set<String> nameServerSet = null;
if (StringUtils.isNotBlank(configure.getNamesrvAddr())) {
String[] ns = configure.getNamesrvAddr().split(";");
nameServerSet = new HashSet<>(Arrays.asList(ns));
}
- mqAdminExt.deleteTopicInNameServer(nameServerSet, topic);
+ if (deleteInNsFlag) {
Review comment:
Yes, these lines of code is covered . The reason for the decrease in coverage this time is that the code modified by this patch(#59) is not covered. Since it is a different branch, the single test cannot be directly added this time, and it will be added in the subsequent submit
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777853679
##########
File path: src/test/java/org/apache/rocketmq/dashboard/controller/ConsumerControllerTest.java
##########
@@ -182,6 +182,8 @@ public void testDelete() throws Exception {
final String url = "/consumer/deleteSubGroup.do";
{
doNothing().when(mqAdminExt).deleteSubscriptionGroup(any(), anyString());
+ doNothing().when(mqAdminExt).deleteTopicInBroker(any(), anyString());
Review comment:
So, how do we know we have deleted what we wanted topic, do we need to check here?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778623813
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -312,14 +320,16 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
return true;
}
- private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo) throws Exception {
+ private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo, boolean deleteInNsFlag) throws Exception {
mqAdminExt.deleteTopicInBroker(Sets.newHashSet(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr()), topic);
Set<String> nameServerSet = null;
if (StringUtils.isNotBlank(configure.getNamesrvAddr())) {
String[] ns = configure.getNamesrvAddr().split(";");
nameServerSet = new HashSet<>(Arrays.asList(ns));
}
- mqAdminExt.deleteTopicInNameServer(nameServerSet, topic);
+ if (deleteInNsFlag) {
Review comment:
Got it
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777874278
##########
File path: src/test/java/org/apache/rocketmq/dashboard/controller/ConsumerControllerTest.java
##########
@@ -182,6 +182,8 @@ public void testDelete() throws Exception {
final String url = "/consumer/deleteSubGroup.do";
{
doNothing().when(mqAdminExt).deleteSubscriptionGroup(any(), anyString());
+ doNothing().when(mqAdminExt).deleteTopicInBroker(any(), anyString());
Review comment:
No need here，mock these two delete interfaces indicate that the topic has been deleted successfully on the broker side, the unit test only need to care about the code logic in the dashboard before and after the deletion.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778203863
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -303,6 +312,16 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
return true;
}
+ private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo) throws Exception {
+ mqAdminExt.deleteTopicInBroker(Sets.newHashSet(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr()), topic);
+ Set<String> nameServerSet = null;
+ if (StringUtils.isNotBlank(configure.getNamesrvAddr())) {
+ String[] ns = configure.getNamesrvAddr().split(";");
+ nameServerSet = new HashSet<>(Arrays.asList(ns));
+ }
+ mqAdminExt.deleteTopicInNameServer(nameServerSet, topic);
Review comment:
Yes, that's a good question, I will fix it in next commit.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777818033
##########
File path: src/test/java/org/apache/rocketmq/dashboard/controller/ConsumerControllerTest.java
##########
@@ -182,6 +182,8 @@ public void testDelete() throws Exception {
final String url = "/consumer/deleteSubGroup.do";
{
doNothing().when(mqAdminExt).deleteSubscriptionGroup(any(), anyString());
+ doNothing().when(mqAdminExt).deleteTopicInBroker(any(), anyString());
Review comment:
In unit tests, all RPC interfaces involved in interacting with RocketMQ used mocks for data return，and this two delete interface methods return void
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] StyleTang commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
StyleTang commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778113789
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -303,6 +312,16 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
return true;
}
+ private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo) throws Exception {
+ mqAdminExt.deleteTopicInBroker(Sets.newHashSet(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr()), topic);
+ Set<String> nameServerSet = null;
+ if (StringUtils.isNotBlank(configure.getNamesrvAddr())) {
+ String[] ns = configure.getNamesrvAddr().split(";");
+ nameServerSet = new HashSet<>(Arrays.asList(ns));
+ }
+ mqAdminExt.deleteTopicInNameServer(nameServerSet, topic);
Review comment:
Just want to confirm whether there is a problem with the `deleteTopicInNameServer`
A SubGroup can exist in multiple brokers.
For example :
GROUP_1
|----- brokerA
|------brokerB
If we just want to delete GROUP_1 in brokerA, deleteTopicInNameServer will also be invoked. It will delete all the QueueData of this topic in namesvr, not sure whether it will cause issue or not.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] vongosling commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
vongosling commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778490497
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -296,14 +296,22 @@ public boolean apply(MessageQueue o) {
@Override
public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
+ Set<String> brokerSet = this.fetchBrokerNameSetBySubscriptionGroup(deleteSubGroupRequest.getGroupName());
+ List<String> brokerList = deleteSubGroupRequest.getBrokerNameList();
+ boolean deleteInNsFlag = false;
+ // If the list of brokers passed in by the request contains the list of brokers that the consumer is in,
+ // delete RETRY and DLQ topic in namesrv
Review comment:
//Delete... The first letter of the comment is capitalized and the style is consistent.
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -312,14 +320,16 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
return true;
}
- private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo) throws Exception {
+ private void deleteResources(String topic, String brokerName, ClusterInfo clusterInfo, boolean deleteInNsFlag) throws Exception {
mqAdminExt.deleteTopicInBroker(Sets.newHashSet(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr()), topic);
Set<String> nameServerSet = null;
if (StringUtils.isNotBlank(configure.getNamesrvAddr())) {
String[] ns = configure.getNamesrvAddr().split(";");
nameServerSet = new HashSet<>(Arrays.asList(ns));
}
- mqAdminExt.deleteTopicInNameServer(nameServerSet, topic);
+ if (deleteInNsFlag) {
Review comment:
Any unit test coverage for this logic?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r777818985
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -295,6 +301,9 @@ public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
for (String brokerName : deleteSubGroupRequest.getBrokerNameList()) {
logger.info("addr={} groupName={}", clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr(), deleteSubGroupRequest.getGroupName());
mqAdminExt.deleteSubscriptionGroup(clusterInfo.getBrokerAddrTable().get(brokerName).selectBrokerAddr(), deleteSubGroupRequest.getGroupName(), true);
+ // delete %RETRY%+Group and %DLQ%+Group in broker and namesrv
+ deleteDlqOrRetryTopic(MixAll.RETRY_GROUP_TOPIC_PREFIX + deleteSubGroupRequest.getGroupName(), brokerName, clusterInfo);
+ deleteDlqOrRetryTopic(MixAll.DLQ_GROUP_TOPIC_PREFIX + deleteSubGroupRequest.getGroupName(), brokerName, clusterInfo);
Review comment:
done
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-dashboard] zhangjidi2016 commented on a change in pull request #57: [ISSUE #55]Delete the corresponding DLQ and Retry Topic simultaneously when deleting the consumerGroup.
Posted by GitBox <gi...@apache.org>.
zhangjidi2016 commented on a change in pull request #57:
URL: https://github.com/apache/rocketmq-dashboard/pull/57#discussion_r778496193
##########
File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/ConsumerServiceImpl.java
##########
@@ -296,14 +296,22 @@ public boolean apply(MessageQueue o) {
@Override
public boolean deleteSubGroup(DeleteSubGroupRequest deleteSubGroupRequest) {
+ Set<String> brokerSet = this.fetchBrokerNameSetBySubscriptionGroup(deleteSubGroupRequest.getGroupName());
+ List<String> brokerList = deleteSubGroupRequest.getBrokerNameList();
+ boolean deleteInNsFlag = false;
+ // If the list of brokers passed in by the request contains the list of brokers that the consumer is in,
+ // delete RETRY and DLQ topic in namesrv
Review comment:
done
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org