You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by Jaskey <gi...@git.apache.org> on 2017/06/07 07:08:45 UTC
[GitHub] incubator-rocketmq pull request #115: [Rocketmq 204]-when all brokers is off...
GitHub user Jaskey opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/115
[Rocketmq 204]-when all brokers is offline, client still attempts to send heartbeat
JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-204?jql=project%20%3D%20ROCKETMQ
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-204-heartbeat-issue-when-topicRouteInfo-becomes-nonexist
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/115.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #115
----
commit 04ec90243f77347e879f21a1ac4ce0e7b3f947b4
Author: Jaskey <li...@gmail.com>
Date: 2017-06-07T07:04:47Z
add unit test to show the problem of topic route info becomes non-exist
commit 7f60c72c9a22c498b8b36072903ae8ac28d8a503
Author: Jaskey <li...@gmail.com>
Date: 2017-06-07T07:06:11Z
[ROCKETMQ-204]-fix the bug that when all brokers is offline, client still attempts to send
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #115: [ROCKETMQ-204]-when all brokers is off...
Posted by vsair <gi...@git.apache.org>.
Github user vsair commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/115#discussion_r121035368
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/factory/MQClientInstance.java ---
@@ -621,34 +621,23 @@ public boolean updateTopicRouteInfoFromNameServer(final String topic, boolean is
{
TopicPublishInfo publishInfo = topicRouteData2TopicPublishInfo(topic, topicRouteData);
publishInfo.setHaveTopicRouterInfo(true);
- Iterator<Entry<String, MQProducerInner>> it = this.producerTable.entrySet().iterator();
- while (it.hasNext()) {
- Entry<String, MQProducerInner> entry = it.next();
- MQProducerInner impl = entry.getValue();
- if (impl != null) {
- impl.updateTopicPublishInfo(topic, publishInfo);
- }
- }
+ updatePubInfoTable(topic, publishInfo);
}
// Update sub info
{
Set<MessageQueue> subscribeInfo = topicRouteData2TopicSubscribeInfo(topic, topicRouteData);
- Iterator<Entry<String, MQConsumerInner>> it = this.consumerTable.entrySet().iterator();
- while (it.hasNext()) {
- Entry<String, MQConsumerInner> entry = it.next();
- MQConsumerInner impl = entry.getValue();
- if (impl != null) {
- impl.updateTopicSubscribeInfo(topic, subscribeInfo);
- }
- }
+ updateSubInfoTable(topic, subscribeInfo);
}
log.info("topicRouteTable.put. Topic = {}, TopicRouteData[{}]", topic, cloneTopicRouteData);
this.topicRouteTable.put(topic, cloneTopicRouteData);
return true;
}
} else {
- log.warn("updateTopicRouteInfoFromNameServer, getTopicRouteInfoFromNameServer return null, Topic: {}", topic);
+ log.warn("updateTopicRouteInfoFromNameServer, getTopicRouteInfoFromNameServer return null. Topic: {}", topic);
--- End diff --
IMO, API(getTopicRouteInfoFromNameServer) will throw exception when not found topic route, so the logic will never enter 'else', am I right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #115: [Rocketmq 204]-when all brokers is offline, c...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/115
[](https://coveralls.io/builds/11862612)
Coverage increased (+0.08%) to 38.692% when pulling **7f60c72c9a22c498b8b36072903ae8ac28d8a503 on Jaskey:ROCKETMQ-204-heartbeat-issue-when-topicRouteInfo-becomes-nonexist** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #115: [ROCKETMQ-204]-when all brokers is off...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/115#discussion_r120857815
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/consumer/DefaultMQPullConsumerImpl.java ---
@@ -324,11 +324,16 @@ public void persistConsumerOffset() {
@Override
public void updateTopicSubscribeInfo(String topic, Set<MessageQueue> info) {
- Map<String, SubscriptionData> subTable = this.rebalanceImpl.getSubscriptionInner();
- if (subTable != null) {
- if (subTable.containsKey(topic)) {
- this.rebalanceImpl.getTopicSubscribeInfoTable().put(topic, info);
+ if (info != null) {
+ Map<String, SubscriptionData> subTable = this.rebalanceImpl.getSubscriptionInner();
+ if (subTable != null) {
+ if (subTable.containsKey(topic)) {
+ this.rebalanceImpl.getTopicSubscribeInfoTable().put(topic, info);
+ }
}
+ } else {
+ Set<MessageQueue> prev = this.rebalanceImpl.getTopicSubscribeInfoTable().remove(topic);
+ log.info("instanceName={}, group={}, topicSubscribeInfoTable of topic {} is remove, {}, prev = {}", defaultMQPullConsumer.getInstanceName(), defaultMQPullConsumer.getConsumerGroup(), topic, prev);
--- End diff --
is remove --> is removed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #115: [ROCKETMQ-204]-when all brokers is offline, c...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/115
@lizhanhui @vsair @zhouxinyu
any opinions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #115: [ROCKETMQ-204]-when all brokers is offline, c...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/115
@Jaskey I'll check it today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #115: [Rocketmq 204]-when all brokers is offline, c...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/115
[](https://coveralls.io/builds/11862708)
Coverage increased (+0.3%) to 38.894% when pulling **0baa2d408799946e4fcd58d4d609b4c6b2ef9a42 on Jaskey:ROCKETMQ-204-heartbeat-issue-when-topicRouteInfo-becomes-nonexist** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #115: [ROCKETMQ-204]-when all brokers is offline, c...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/115
[](https://coveralls.io/builds/11897988)
Coverage decreased (-0.03%) to 38.579% when pulling **aa59bbdab68966257204fd9d44b099f43ba6dfb6 on Jaskey:ROCKETMQ-204-heartbeat-issue-when-topicRouteInfo-becomes-nonexist** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #115: [ROCKETMQ-204]-when all brokers is off...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/115#discussion_r121039751
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/factory/MQClientInstance.java ---
@@ -621,34 +621,23 @@ public boolean updateTopicRouteInfoFromNameServer(final String topic, boolean is
{
TopicPublishInfo publishInfo = topicRouteData2TopicPublishInfo(topic, topicRouteData);
publishInfo.setHaveTopicRouterInfo(true);
- Iterator<Entry<String, MQProducerInner>> it = this.producerTable.entrySet().iterator();
- while (it.hasNext()) {
- Entry<String, MQProducerInner> entry = it.next();
- MQProducerInner impl = entry.getValue();
- if (impl != null) {
- impl.updateTopicPublishInfo(topic, publishInfo);
- }
- }
+ updatePubInfoTable(topic, publishInfo);
}
// Update sub info
{
Set<MessageQueue> subscribeInfo = topicRouteData2TopicSubscribeInfo(topic, topicRouteData);
- Iterator<Entry<String, MQConsumerInner>> it = this.consumerTable.entrySet().iterator();
- while (it.hasNext()) {
- Entry<String, MQConsumerInner> entry = it.next();
- MQConsumerInner impl = entry.getValue();
- if (impl != null) {
- impl.updateTopicSubscribeInfo(topic, subscribeInfo);
- }
- }
+ updateSubInfoTable(topic, subscribeInfo);
}
log.info("topicRouteTable.put. Topic = {}, TopicRouteData[{}]", topic, cloneTopicRouteData);
this.topicRouteTable.put(topic, cloneTopicRouteData);
return true;
}
} else {
- log.warn("updateTopicRouteInfoFromNameServer, getTopicRouteInfoFromNameServer return null, Topic: {}", topic);
+ log.warn("updateTopicRouteInfoFromNameServer, getTopicRouteInfoFromNameServer return null. Topic: {}", topic);
--- End diff --
It seems you are right, this design is confusing , maybe we change the logic into returns null?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---