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/08/04 03:00:19 UTC

[GitHub] [rocketmq] xijiu opened a new pull request, #4772: [ISSUE #3870] Optimize topic route data notification

xijiu opened a new pull request, #4772:
URL: https://github.com/apache/rocketmq/pull/4772

   **Make sure set the target branch to `develop`**
   
   ## What is the purpose of the change
   
   Optimize topic route data notification
   [Github issue 3870](https://github.com/apache/rocketmq/issues/3870)
   
   ## Brief changelog
   
   * NameServer
   * Broker
   * Client
   
   


-- 
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

Re: [PR] [ISSUE #3870] Optimize topic route data notification [rocketmq]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4772:
URL: https://github.com/apache/rocketmq/pull/4772#issuecomment-1780220768

   This PR is stale because it has been open for 365 days with no activity. It will be closed in 3 days if no further activity occurs. If you wish not to mark it as stale, please leave a comment in this 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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] dongeforever commented on a diff in pull request #4772: [ISSUE #3870] Optimize topic route data notification

Posted by GitBox <gi...@apache.org>.
dongeforever commented on code in PR #4772:
URL: https://github.com/apache/rocketmq/pull/4772#discussion_r990617511


##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/TopicRouteNotifier.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.rocketmq.namesrv.routeinfo;
+
+import io.netty.channel.Channel;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.collections.MapUtils;
+import org.apache.rocketmq.common.constant.LoggerName;
+import org.apache.rocketmq.common.protocol.RequestCode;
+import org.apache.rocketmq.common.protocol.header.namesrv.UpdateTopicRouteRequestHeader;
+import org.apache.rocketmq.logging.InternalLogger;
+import org.apache.rocketmq.logging.InternalLoggerFactory;
+import org.apache.rocketmq.remoting.RemotingServer;
+import org.apache.rocketmq.remoting.protocol.RemotingCommand;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * if topic route info changed, then notify client scheduled
+ */
+public class TopicRouteNotifier {
+    private static final InternalLogger log = InternalLoggerFactory.getLogger(LoggerName.NAMESRV_LOGGER_NAME);
+
+    private final RouteInfoManager routeInfoManager;
+
+    private final RemotingServer remotingServer;
+
+    public TopicRouteNotifier(RemotingServer remotingServer, RouteInfoManager routeInfoManager) {
+        this.routeInfoManager = routeInfoManager;
+        this.remotingServer = remotingServer;
+    }
+
+    /**
+     * if topic route info has changed in the period, then notify client
+     */
+    public void notifyClients() {
+        Map<String, Set<Channel>> topicAndChannelMap = routeInfoManager.getAndResetChangedTopicMap();
+        if (MapUtils.isEmpty(topicAndChannelMap)) {

Review Comment:
   here may need a dynamic config to control whether to notify or not.



##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java:
##########
@@ -455,6 +470,10 @@ private void createAndUpdateQueueData(final String brokerName, final TopicConfig
                 log.info("topic changed, {} OLD: {} NEW: {}", topicConfig.getTopicName(), existedQD,
                     queueData);
                 queueDataMap.put(brokerName, queueData);
+                // if broker restart, many topic will register, for avoid hot, then ignore
+                if (singleTopicRouteChanged) {

Review Comment:
   In case of "broker restart/failure", it also needs to be notified.
   
   How about using the multi-node to improve performance and reduce CPU cost?
   
   
   
   



##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/TopicRouteNotifier.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.rocketmq.namesrv.routeinfo;
+
+import io.netty.channel.Channel;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.collections.MapUtils;
+import org.apache.rocketmq.common.constant.LoggerName;
+import org.apache.rocketmq.common.protocol.RequestCode;
+import org.apache.rocketmq.common.protocol.header.namesrv.UpdateTopicRouteRequestHeader;
+import org.apache.rocketmq.logging.InternalLogger;
+import org.apache.rocketmq.logging.InternalLoggerFactory;
+import org.apache.rocketmq.remoting.RemotingServer;
+import org.apache.rocketmq.remoting.protocol.RemotingCommand;
+
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * if topic route info changed, then notify client scheduled
+ */
+public class TopicRouteNotifier {
+    private static final InternalLogger log = InternalLoggerFactory.getLogger(LoggerName.NAMESRV_LOGGER_NAME);
+
+    private final RouteInfoManager routeInfoManager;
+
+    private final RemotingServer remotingServer;
+
+    public TopicRouteNotifier(RemotingServer remotingServer, RouteInfoManager routeInfoManager) {
+        this.routeInfoManager = routeInfoManager;
+        this.remotingServer = remotingServer;
+    }
+
+    /**
+     * if topic route info has changed in the period, then notify client
+     */
+    public void notifyClients() {
+        Map<String, Set<Channel>> topicAndChannelMap = routeInfoManager.getAndResetChangedTopicMap();
+        if (MapUtils.isEmpty(topicAndChannelMap)) {
+            return;
+        }
+
+        for (Map.Entry<String, Set<Channel>> entry : topicAndChannelMap.entrySet()) {
+            notifyClientsByTopic(entry.getKey(), entry.getValue());
+        }
+    }
+
+    private void notifyClientsByTopic(String topic, Set<Channel> channelSet) {
+        if (topic == null || CollectionUtils.isEmpty(channelSet)) {
+            return;
+        }
+        for (Channel channel : channelSet) {
+            RemotingCommand remotingCommand = transToCommand(topic);
+            try {
+                remotingServer.invokeOneway(channel, remotingCommand, 50);
+            } catch (Exception e) {
+                log.error("invoke client exception. topic={}, channel={}, error={}", topic, channel, e.toString());
+            }
+        }
+    }
+

Review Comment:
   Here may need to support multi-topic mode,  that is to notify the channel multi topics at one time.
   
   Maybe the getTopicRoute needs to support multi-mode too.



-- 
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] codecov-commenter commented on pull request #4772: [ISSUE #3870] Optimize topic route data notification

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4772:
URL: https://github.com/apache/rocketmq/pull/4772#issuecomment-1259147553

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4772?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#4772](https://codecov.io/gh/apache/rocketmq/pull/4772?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dcc8eda) into [develop](https://codecov.io/gh/apache/rocketmq/commit/590a680c38f6aaf8f01581503a6338f08256914e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (590a680) will **increase** coverage by `0.01%`.
   > The diff coverage is `60.00%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #4772      +/-   ##
   =============================================
   + Coverage      43.25%   43.26%   +0.01%     
   - Complexity      7799     7803       +4     
   =============================================
     Files            997      997              
     Lines          69350    69362      +12     
     Branches        9166     9161       -5     
   =============================================
   + Hits           29994    30008      +14     
   - Misses         35582    35588       +6     
   + Partials        3774     3766       -8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4772?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../java/org/apache/rocketmq/acl/common/AclUtils.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YWNsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9hY2wvY29tbW9uL0FjbFV0aWxzLmphdmE=) | `70.92% <60.00%> (-0.21%)` | :arrow_down: |
   | [...n/java/org/apache/rocketmq/test/util/StatUtil.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGVzdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvdGVzdC91dGlsL1N0YXRVdGlsLmphdmE=) | `16.19% <0.00%> (-14.09%)` | :arrow_down: |
   | [...q/namesrv/routeinfo/BrokerHousekeepingService.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bmFtZXNydi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbmFtZXNydi9yb3V0ZWluZm8vQnJva2VySG91c2VrZWVwaW5nU2VydmljZS5qYXZh) | `72.72% <0.00%> (-9.10%)` | :arrow_down: |
   | [...va/org/apache/rocketmq/logging/inner/Appender.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9BcHBlbmRlci5qYXZh) | `29.21% <0.00%> (-7.87%)` | :arrow_down: |
   | [...ache/rocketmq/container/BrokerContainerConfig.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udGFpbmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb250YWluZXIvQnJva2VyQ29udGFpbmVyQ29uZmlnLmphdmE=) | `92.00% <0.00%> (-3.24%)` | :arrow_down: |
   | [...che/rocketmq/namesrv/kvconfig/KVConfigManager.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bmFtZXNydi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbmFtZXNydi9rdmNvbmZpZy9LVkNvbmZpZ01hbmFnZXIuamF2YQ==) | `62.36% <0.00%> (-1.08%)` | :arrow_down: |
   | [...org/apache/rocketmq/container/BrokerContainer.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udGFpbmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb250YWluZXIvQnJva2VyQ29udGFpbmVyLmphdmE=) | `53.73% <0.00%> (-1.03%)` | :arrow_down: |
   | [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `38.12% <0.00%> (-0.56%)` | :arrow_down: |
   | [...e/rocketmq/namesrv/routeinfo/RouteInfoManager.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bmFtZXNydi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbmFtZXNydi9yb3V0ZWluZm8vUm91dGVJbmZvTWFuYWdlci5qYXZh) | `63.69% <0.00%> (-0.33%)` | :arrow_down: |
   | [...a/org/apache/rocketmq/broker/BrokerController.java](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvQnJva2VyQ29udHJvbGxlci5qYXZh) | `43.67% <0.00%> (-0.18%)` | :arrow_down: |
   | ... and [32 more](https://codecov.io/gh/apache/rocketmq/pull/4772/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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] dongeforever commented on a diff in pull request #4772: [ISSUE #3870] Optimize topic route data notification

Posted by GitBox <gi...@apache.org>.
dongeforever commented on code in PR #4772:
URL: https://github.com/apache/rocketmq/pull/4772#discussion_r990619581


##########
client/src/main/java/org/apache/rocketmq/client/impl/producer/DefaultMQProducerImpl.java:
##########
@@ -738,19 +768,22 @@ private SendResult sendDefaultImpl(
 
     private TopicPublishInfo tryToFindTopicPublishInfo(final String topic) {
         TopicPublishInfo topicPublishInfo = this.topicPublishInfoTable.get(topic);
+
+        // topic route info not in local cache, then make a request to nameServer
         if (null == topicPublishInfo || !topicPublishInfo.ok()) {
             this.topicPublishInfoTable.putIfAbsent(topic, new TopicPublishInfo());

Review Comment:
   If the topicPublishInfo is not ok(i.e. the topic does not exist), it will generate RPC for each message, which will cause too many requests to the nameserver, and make the nameserver to be hot.
   
   For the failed topic,  we may need to add a placeholder for 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

Re: [PR] [ISSUE #3870] Optimize topic route data notification [rocketmq]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4772: [ISSUE #3870] Optimize topic route data notification
URL: https://github.com/apache/rocketmq/pull/4772


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ISSUE #3870] Optimize topic route data notification [rocketmq]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4772:
URL: https://github.com/apache/rocketmq/pull/4772#issuecomment-1783947831

   This PR was closed because it has been inactive for 3 days since being marked as stale.


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org