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/02/14 06:38:03 UTC
[GitHub] incubator-rocketmq pull request #60: [ROCKETMQ-96]Rename some temp variable ...
GitHub user Jaskey opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/60
[ROCKETMQ-96]Rename some temp variable and field
JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-96
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-96
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/60.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 #60
----
commit 7455bdf093762c5ab248d18e0788d3c42d6639b5
Author: Jaskey <li...@gmail.com>
Date: 2017-02-14T06:37:24Z
rename some temp variable and field
----
---
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 #60: [ROCKETMQ-96]Rename some temp variable ...
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/60#discussion_r105594848
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/producer/DefaultMQProducerImpl.java ---
@@ -449,9 +449,9 @@ private SendResult sendDefaultImpl(//
String[] brokersSent = new String[timesTotal];
for (; times < timesTotal; times++) {
String lastBrokerName = null == mq ? null : mq.getBrokerName();
- MessageQueue tmpmq = this.selectOneMessageQueue(topicPublishInfo, lastBrokerName);
- if (tmpmq != null) {
- mq = tmpmq;
+ MessageQueue mqAttempt = this.selectOneMessageQueue(topicPublishInfo, lastBrokerName);
--- End diff --
IMO , any variable named like XXXtmp is bad. Maybe we can rename it to another one but tmpMq is far from good enough
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
@shroman @lizhanhui @zhouxinyu
There are users asking the same question about the logic of consume orderly and they both complain that the `msgTreeMapTemp` is very hard to understand it's implication, so would you please review this pr and merge it in 4.1.
Besides, I wonder if there is the same issue in below method, the signature of the method is trying to determine if there are some tmp msg(which will be only used in consumer orderly service), while the implementation is to determine the `msgTreeMap`?
Is the implementation wrong or the is signature misleading.
public boolean hasTempMessage() {
try {
this.lockTreeMap.readLock().lockInterruptibly();
try {
return !this.msgTreeMap.isEmpty();
} finally {
this.lockTreeMap.readLock().unlock();
}
} catch (InterruptedException e) {
}
return true;
}
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
@zhouxinyu @shroman @vongosling @lizhanhui
Guys, how do you think of this pr, which does not change any functionality , I just rename some variables/fields to make code more readable.
---
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 #60: [ROCKETMQ-96]Rename some temp variable ...
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/60#discussion_r105590475
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/consumer/ProcessQueue.java ---
@@ -46,7 +46,7 @@
private final TreeMap<Long, MessageExt> msgTreeMap = new TreeMap<Long, MessageExt>();
private final AtomicLong msgCount = new AtomicLong();
private final Lock lockConsume = new ReentrantLock();
- private final TreeMap<Long, MessageExt> msgTreeMapTemp = new TreeMap<Long, MessageExt>();
+ private final TreeMap<Long, MessageExt> consumingMsgOrderlyTreeMap = new TreeMap<Long, MessageExt>();//subset of msgTreeMap, used when consume orderly
--- End diff --
It would be nice to have a Javadoc comment here. Something like,
```java
/**
* Subset of msgTreeMap used when consuming orderly.
*/
private final TreeMap<Long, MessageExt> msgOrderlyTreeMap = ...
```
---
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 #60: [ROCKETMQ-96]Rename some temp variable ...
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/60#discussion_r105590248
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/producer/DefaultMQProducerImpl.java ---
@@ -449,9 +449,9 @@ private SendResult sendDefaultImpl(//
String[] brokersSent = new String[timesTotal];
for (; times < timesTotal; times++) {
String lastBrokerName = null == mq ? null : mq.getBrokerName();
- MessageQueue tmpmq = this.selectOneMessageQueue(topicPublishInfo, lastBrokerName);
- if (tmpmq != null) {
- mq = tmpmq;
+ MessageQueue mqAttempt = this.selectOneMessageQueue(topicPublishInfo, lastBrokerName);
--- End diff --
IMO, `mqAttempt` does not make the variable better. `tmpMq` is good enough here.
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
@Jaskey No idea about `hasTempMessage()` -- no comments or unit tests...
All the rest is ok to merge.
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
+1 to merge it.
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
@zhouxinyu @vongosling
Any comments on this pr and any ideas about `hasTempMessage()`?
---
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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
[](https://coveralls.io/builds/10143266)
Coverage increased (+0.02%) to 31.542% when pulling **7455bdf093762c5ab248d18e0788d3c42d6639b5 on Jaskey:ROCKETMQ-96** into **573b22c37806a21543b90707bcce6022243a62da 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 #60: [ROCKETMQ-96]Rename some temp variable and fie...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/60
[](https://coveralls.io/builds/11470474)
Coverage decreased (-0.05%) to 37.865% when pulling **ad82f0215e97bcad0b40379a6ba0b1a965b4d904 on Jaskey:ROCKETMQ-96** into **80aac138d905561c7a63c8e15fdfe60e958a3032 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 pull request #60: [ROCKETMQ-96]Rename some temp variable ...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/60#discussion_r102896928
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/consumer/ProcessQueue.java ---
@@ -46,7 +46,7 @@
private final TreeMap<Long, MessageExt> msgTreeMap = new TreeMap<Long, MessageExt>();
private final AtomicLong msgCount = new AtomicLong();
private final Lock lockConsume = new ReentrantLock();
- private final TreeMap<Long, MessageExt> msgTreeMapTemp = new TreeMap<Long, MessageExt>();
+ private final TreeMap<Long, MessageExt> consumingMsgOrderlyTreeMap = new TreeMap<Long, MessageExt>();//subset of msgTreeMap, used when consume orderly
--- End diff --
Hi, in this PR, what's the difference between `msgTreeMap` and `consumingMsgOrderlyTreeMap`?
---
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 #60: [ROCKETMQ-96]Rename some temp variable ...
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/60#discussion_r102897698
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/consumer/ProcessQueue.java ---
@@ -46,7 +46,7 @@
private final TreeMap<Long, MessageExt> msgTreeMap = new TreeMap<Long, MessageExt>();
private final AtomicLong msgCount = new AtomicLong();
private final Lock lockConsume = new ReentrantLock();
- private final TreeMap<Long, MessageExt> msgTreeMapTemp = new TreeMap<Long, MessageExt>();
+ private final TreeMap<Long, MessageExt> consumingMsgOrderlyTreeMap = new TreeMap<Long, MessageExt>();//subset of msgTreeMap, used when consume orderly
--- End diff --
I do not change anything except the variable, this PR is to solve that it is very hard to know what's the purpose of `msgTreeMapTemp `, when I review the code, I think it is only for order service, so I rename the field.
The difference between `msgTreeMap` and `consumingMsgOrderlyTreeMap` is that , `consumingMsgOrderlyTreeMap` is the subset of `msgTreeMap` when using consumeOrderlyService , msg will only be putinto `consumingMsgOrderlyTreeMap` when they are ready to submit to thread pool.
Actually it takes me more than ten minutes to figure out that, that's why I issue this pr.
---
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.
---