Skip to content

Conversation

sunxi92
Copy link
Contributor

@sunxi92 sunxi92 commented May 9, 2022

@coveralls
Copy link

coveralls commented May 9, 2022

Coverage Status

Coverage decreased (-0.06%) to 52.057% when pulling 6969a99 on sunxi92:delete-topic-route-based-on-cluster into 5d2bf35 on apache:develop.

@ni-ze
Copy link
Contributor

ni-ze commented May 10, 2022

Is the purpose of this pr to support multiple clusterNames?
otherwise, the code is very similar to the original, I think, the same parts should be reused as much as possible.

Copy link
Contributor

@lwclover lwclover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In DeleteTopicSubCommand, The parameter 'clusterName' is mandatory. So deleting a topic with the parameters 'topic' and 'clusterName' is clear.

@sunxi92
Copy link
Contributor Author

sunxi92 commented May 12, 2022

In DeleteTopicSubCommand, The parameter 'clusterName' is mandatory. So deleting a topic with the parameters 'topic' and 'clusterName' is clear.

DeleteTopicSubCommand should delete topic route info based on clusterName in nameserver. But the processing logic of nameserver is deleting all the topic route info in topicQueueTable before updating the code. @lwclover

@lwclover
Copy link
Contributor

lwclover commented May 13, 2022

eleteTopicSubCommand should delete topic route info based on clusterNa

This is an administrative command. In my opion, You don't have to write compatibility code.

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022

Codecov Report

Merging #4268 (5f39699) into develop (4574d59) will increase coverage by 0.14%.
The diff coverage is 23.07%.

@@              Coverage Diff              @@
##             develop    #4268      +/-   ##
=============================================
+ Coverage      47.90%   48.04%   +0.14%     
- Complexity      5001     5065      +64     
=============================================
  Files            633      642       +9     
  Lines          42523    42790     +267     
  Branches        5573     5604      +31     
=============================================
+ Hits           20371    20559     +188     
- Misses         19654    19707      +53     
- Partials        2498     2524      +26     
Impacted Files Coverage Δ
...g/apache/rocketmq/client/impl/MQClientAPIImpl.java 13.96% <ø> (+0.10%) ⬆️
...r/namesrv/DeleteTopicFromNamesrvRequestHeader.java 0.00% <0.00%> (ø)
...apache/rocketmq/tools/admin/DefaultMQAdminExt.java 41.40% <0.00%> (ø)
...he/rocketmq/tools/admin/DefaultMQAdminExtImpl.java 39.83% <0.00%> (ø)
...tmq/tools/command/topic/DeleteTopicSubCommand.java 27.27% <0.00%> (ø)
...e/rocketmq/namesrv/routeinfo/RouteInfoManager.java 78.83% <26.31%> (-2.69%) ⬇️
...tmq/namesrv/processor/DefaultRequestProcessor.java 74.28% <100.00%> (ø)
...va/org/apache/rocketmq/store/FlushDiskWatcher.java 81.25% <0.00%> (-9.38%) ⬇️
...broker/processor/AbstractSendMessageProcessor.java 40.62% <0.00%> (-8.91%) ⬇️
...he/rocketmq/remoting/protocol/RemotingCommand.java 72.45% <0.00%> (-6.18%) ⬇️
... and 80 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4574d59...5f39699. Read the comment docs.

2.move the judgment of whether there are any key-value pairs in queueDataMap out of the for loop
@lwclover
Copy link
Contributor

lwclover commented May 17, 2022

The new method need testcase.

Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @sunxi92 , IMO, compatibility needs to be considered here. If clusterName exists, delete it based on the clusterName. Otherwise, delete the whole topic. Otherwise, there will be issues with older versions of the admin tool or console calls.

@lwclover
Copy link
Contributor

Hi, @sunxi92 , IMO, compatibility needs to be considered here. If clusterName exists, delete it based on the clusterName. Otherwise, delete the whole topic. Otherwise, there will be issues with older versions of the admin tool or console calls.

@sunxi92 I'm sorry I told you I removed the compatibility code.

@sunxi92
Copy link
Contributor Author

sunxi92 commented May 17, 2022

Hi, @sunxi92 , IMO, compatibility needs to be considered here. If clusterName exists, delete it based on the clusterName. Otherwise, delete the whole topic. Otherwise, there will be issues with older versions of the admin tool or console calls.

done!

@RongtongJin RongtongJin merged commit b1c5fef into apache:develop Jun 6, 2022
@RongtongJin RongtongJin added this to the 4.9.4 milestone Jun 6, 2022
@duhenglucky duhenglucky changed the title [ISSUE#4263]Delete topic route info based on cluster when delete topic. [ISSUE #4263]Delete topic route info based on cluster when delete topic. Jun 6, 2022
madongming1001 pushed a commit to madongming1001/rocketmq that referenced this pull request Jun 6, 2022
…ic. (apache#4268)

* Delete topic route info based on cluster when delete topic.

* Delete useless compatibility code.

* 1.add a non-null judgment to queueDataMap
2.move the judgment of whether there are any key-value pairs in queueDataMap out of the for loop

* When clusterName is null, then delete topic route info in namesrv.
GenerousMan pushed a commit to GenerousMan/rocketmq that referenced this pull request Aug 12, 2022
…ic. (apache#4268)

* Delete topic route info based on cluster when delete topic.

* Delete useless compatibility code.

* 1.add a non-null judgment to queueDataMap
2.move the judgment of whether there are any key-value pairs in queueDataMap out of the for loop

* When clusterName is null, then delete topic route info in namesrv.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants