You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by shroman <gi...@git.apache.org> on 2017/07/10 08:29:49 UTC
[GitHub] incubator-rocketmq pull request #130: [ROCKETMQ-243] BrokerData#selectBroker...
GitHub user shroman opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/130
[ROCKETMQ-243] BrokerData#selectBrokerAddr() picks the 1st element in the list of addresses
https://issues.apache.org/jira/browse/ROCKETMQ-243
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shroman/incubator-rocketmq ROCKETMQ-243
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/130.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 #130
----
commit f3a5cbf58858b4fc5565fe1e094303670753be61
Author: shroman <rs...@yahoo.com>
Date: 2017-07-10T08:25:48Z
[ROCKETMQ-243] BrokerData#selectBrokerAddr() picks the 1st element in the list of addresses
https://issues.apache.org/jira/browse/ROCKETMQ-243
----
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
@Jaskey It returns a random broker address if the one with MASTER_ID is not found. Why do you suggest renaming?
I hope the execution flow doesn't depend on the current method implementation and broker address change is ok -- otherwise it is a huge flaw for a distributed system like this. Guys, please share your thoughts.
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
@Jaskey I modified the comment, thanks!
---
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 #130: [ROCKETMQ-243] BrokerData#selectBroker...
Posted by shroman <gi...@git.apache.org>.
Github user shroman closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/130
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
Merged to develop branch.
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
LGTM
---
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 #130: [ROCKETMQ-243] BrokerData#selectBroker...
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/130#discussion_r126865859
--- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/route/BrokerData.java ---
@@ -37,15 +41,21 @@ public BrokerData(String cluster, String brokerName, HashMap<Long, String> broke
this.brokerAddrs = brokerAddrs;
}
+ /**
+ * Selects a (preferably master) broker address from the registered list.
+ * If the master's address cannot be found, a slave broker address is selected in a random manner.
+ *
+ * @return Broker address.
+ */
public String selectBrokerAddr() {
- String value = this.brokerAddrs.get(MixAll.MASTER_ID);
- if (null == value) {
- for (Map.Entry<Long, String> entry : this.brokerAddrs.entrySet()) {
- return entry.getValue();
- }
+ String addr = this.brokerAddrs.get(MixAll.MASTER_ID);
+
+ if (addr == null) {
+ List<Long> keys = new ArrayList<Long>(brokerAddrs.keySet());
+ return brokerAddrs.get(keys.get(random.nextInt(keys.size())));
--- End diff --
@shroman use `values()` will benefit from one less method call of `map.get(i)`
---
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 #130: [ROCKETMQ-243] BrokerData#selectBroker...
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/130#discussion_r126856546
--- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/route/BrokerData.java ---
@@ -37,15 +41,21 @@ public BrokerData(String cluster, String brokerName, HashMap<Long, String> broke
this.brokerAddrs = brokerAddrs;
}
+ /**
+ * Selects a (preferably master) broker address from the registered list.
+ * If the master's address cannot be found, a slave broker address is selected in a random manner.
+ *
+ * @return Broker address.
+ */
public String selectBrokerAddr() {
- String value = this.brokerAddrs.get(MixAll.MASTER_ID);
- if (null == value) {
- for (Map.Entry<Long, String> entry : this.brokerAddrs.entrySet()) {
- return entry.getValue();
- }
+ String addr = this.brokerAddrs.get(MixAll.MASTER_ID);
+
+ if (addr == null) {
+ List<Long> keys = new ArrayList<Long>(brokerAddrs.keySet());
+ return brokerAddrs.get(keys.get(random.nextInt(keys.size())));
--- End diff --
why not use values to construct list directly?
---
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 #130: [ROCKETMQ-243] BrokerData#selectBroker...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/130#discussion_r126867179
--- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/route/BrokerData.java ---
@@ -37,15 +41,21 @@ public BrokerData(String cluster, String brokerName, HashMap<Long, String> broke
this.brokerAddrs = brokerAddrs;
}
+ /**
+ * Selects a (preferably master) broker address from the registered list.
+ * If the master's address cannot be found, a slave broker address is selected in a random manner.
+ *
+ * @return Broker address.
+ */
public String selectBrokerAddr() {
- String value = this.brokerAddrs.get(MixAll.MASTER_ID);
- if (null == value) {
- for (Map.Entry<Long, String> entry : this.brokerAddrs.entrySet()) {
- return entry.getValue();
- }
+ String addr = this.brokerAddrs.get(MixAll.MASTER_ID);
+
+ if (addr == null) {
+ List<Long> keys = new ArrayList<Long>(brokerAddrs.keySet());
+ return brokerAddrs.get(keys.get(random.nextInt(keys.size())));
--- End diff --
Ok, changed to the list.
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
[](https://coveralls.io/builds/12354164)
Coverage increased (+0.7%) to 39.176% when pulling **3f31fc056bd35d0d85568d5ea284a9919e7219d7 on shroman:ROCKETMQ-243** into **246be9eb06a8686435f1e8db1e4fac7a0b737e89 on apache:develop**.
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
@shroman Sorry I misunderstand this method that I though it returns the master address, since it is prefered to return the master address then slave, renaming seems to be not necessary while comment should be modified since it is only randomly pick while master is not found, thus only slave will be randomly pick.
---
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 #130: [ROCKETMQ-243] BrokerData#selectBroker...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/130#discussion_r126858457
--- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/route/BrokerData.java ---
@@ -37,15 +41,21 @@ public BrokerData(String cluster, String brokerName, HashMap<Long, String> broke
this.brokerAddrs = brokerAddrs;
}
+ /**
+ * Selects a (preferably master) broker address from the registered list.
+ * If the master's address cannot be found, a slave broker address is selected in a random manner.
+ *
+ * @return Broker address.
+ */
public String selectBrokerAddr() {
- String value = this.brokerAddrs.get(MixAll.MASTER_ID);
- if (null == value) {
- for (Map.Entry<Long, String> entry : this.brokerAddrs.entrySet()) {
- return entry.getValue();
- }
+ String addr = this.brokerAddrs.get(MixAll.MASTER_ID);
+
+ if (addr == null) {
+ List<Long> keys = new ArrayList<Long>(brokerAddrs.keySet());
+ return brokerAddrs.get(keys.get(random.nextInt(keys.size())));
--- End diff --
what exactly is the problem with the current approach?
---
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 #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/130
Broker contention should be safe but random pick can balance the network call for to all masters.
but we have to be cautions about this since for the current implementations, the broker addr will be very stable since it always return the same broker addr to the caller.
There are more than ten caller methods on this `selectBrokerAddr`, we have to make sure the random pick will not cause any side effect.
I believe it has not @lizhanhui @vongosling @zhouxinyu what's your opions.
@shroman , and since this method only returns the master addrs, I will suggest rename it to `pickMasterAddr`. Since the method is only called by some inner impl caller, I think it is safe to rename.
---
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.
---