You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by Ritabrata-TW <gi...@git.apache.org> on 2017/07/25 16:04:16 UTC
[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...
GitHub user Ritabrata-TW opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/134
Rito - #44 - Extracting out duplicated code from DefaultMessageStore.…
This commit is in reference to Story number 44 from the Kanban Board, which refers to removing duplicated logic from DefaultMessageStore.getEarliestMessageTime(). Please let me know if you need any improvements.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Ritabrata-TW/incubator-rocketmq master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/134.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 #134
----
commit 44e132ba77b284e520b5a538f0cffa78f1aa5ec3
Author: Ritabrata Moitra <rm...@thoughtworks.com>
Date: 2017-07-25T16:02:43Z
Rito - #44 - Extracting out duplicated code from DefaultMessageStore.getEarliestMessageTime
----
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
[](https://coveralls.io/builds/12692862)
Coverage decreased (-0.3%) to 38.78% when pulling **dc2ce20af7715e454bd20f3dae039c2d13bc41cf on Ritabrata-TW:master** into **d4149207e27ed3516f1f06407b55986790b806ae 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 #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r130238369
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -717,22 +718,28 @@ public long getEarliestMessageTime(String topic, int queueId) {
long minLogicOffset = logicQueue.getMinLogicOffset();
SelectMappedBufferResult result = logicQueue.getIndexBuffer(minLogicOffset / ConsumeQueue.CQ_STORE_UNIT_SIZE);
- if (result != null) {
- try {
- final long phyOffset = result.getByteBuffer().getLong();
- final int size = result.getByteBuffer().getInt();
- long storeTime = this.getCommitLog().pickupStoreTimestamp(phyOffset, size);
- return storeTime;
- } catch (Exception e) {
- } finally {
- result.release();
- }
- }
+ Long storeTime = getStoreTime(result);
+ return storeTime;
--- End diff --
`return getStoreTime(result);` will suffice.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by vsair <gi...@git.apache.org>.
Github user vsair commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
Is there something wrong with your code style ?
---
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 #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r130238308
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
+ } else {
+ // TODO LOG
--- End diff --
Please don't leave TODO if possible.
---
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 #134: Rito - #44 - Extracting out duplicated...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r130596160
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -717,22 +718,28 @@ public long getEarliestMessageTime(String topic, int queueId) {
long minLogicOffset = logicQueue.getMinLogicOffset();
SelectMappedBufferResult result = logicQueue.getIndexBuffer(minLogicOffset / ConsumeQueue.CQ_STORE_UNIT_SIZE);
- if (result != null) {
- try {
- final long phyOffset = result.getByteBuffer().getLong();
- final int size = result.getByteBuffer().getInt();
- long storeTime = this.getCommitLog().pickupStoreTimestamp(phyOffset, size);
- return storeTime;
- } catch (Exception e) {
- } finally {
- result.release();
- }
- }
+ Long storeTime = getStoreTime(result);
+ return storeTime;
--- End diff --
Yup. Made the change.
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW Looking forward to your following for 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.
---
[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r129469074
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1199,14 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
- log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ if(allowTopicNotExist) {
--- End diff --
Could you polish the 'double if'?
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@shroman Can you take a look? 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 issue #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@vongosling Sorry for the late response. I have changed the PR description according to the format suggested. I agree to what you said about splitting the big refactor into multiple parts and will keep this in mind for future issues. This PR has commits from 2 issues and it would be a little tedious to pull out these commits into multiple PRs now. I will attach both the issues in the PR description. Please have a look and let me know if this is okay.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in Defa...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/134
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@zhouxinyu Thanks a lot. Will look forward to working with you again.
---
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 #134: Rito - #44 - Extracting out duplicated...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r130526510
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -745,17 +751,7 @@ public long getMessageStoreTimeStamp(String topic, int queueId, long consumeQueu
ConsumeQueue logicQueue = this.findConsumeQueue(topic, queueId);
if (logicQueue != null) {
SelectMappedBufferResult result = logicQueue.getIndexBuffer(consumeQueueOffset);
- if (result != null) {
- try {
- final long phyOffset = result.getByteBuffer().getLong();
- final int size = result.getByteBuffer().getInt();
- long storeTime = this.getCommitLog().pickupStoreTimestamp(phyOffset, size);
- return storeTime;
- } catch (Exception ignored) {
- } finally {
- result.release();
- }
- }
+ long storeTime = getStoreTime(result);
--- End diff --
We appreciate your contribution for rocketmq commuity, could you use unit test to verify your refactoring
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@vongosling @zhouxinyu Anything else that I need to do on this? Otherwise, can you please have a look and 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 pull request #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r129468910
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -717,22 +717,28 @@ public long getEarliestMessageTime(String topic, int queueId) {
long minLogicOffset = logicQueue.getMinLogicOffset();
SelectMappedBufferResult result = logicQueue.getIndexBuffer(minLogicOffset / ConsumeQueue.CQ_STORE_UNIT_SIZE);
- if (result != null) {
- try {
- final long phyOffset = result.getByteBuffer().getLong();
- final int size = result.getByteBuffer().getInt();
- long storeTime = this.getCommitLog().pickupStoreTimestamp(phyOffset, size);
- return storeTime;
- } catch (Exception e) {
- } finally {
- result.release();
- }
- }
+ Long storeTime = getStoreTime(result);
+ if (storeTime != null) return storeTime;
}
return -1;
}
+ private Long getStoreTime(SelectMappedBufferResult result) {
--- End diff --
IMO, just return long and default value is -1.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in Defa...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r131365055
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
--- End diff --
@shroman Ahh. I get you now. Making the change and pushing. 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 issue #134: Rito - #44 - Extracting out duplicated code f...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
[](https://coveralls.io/builds/12638697)
Coverage decreased (-0.3%) to 38.79% when pulling **28e14be3ea8fbe24814bcdebd61b36d94dd5b3b2 on Ritabrata-TW:master** into **d4149207e27ed3516f1f06407b55986790b806ae 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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@vongosling Please have a look and let me know if you need any further improvements about the same. 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 issue #134: Rito - #44 - Extracting out duplicated code f...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
[](https://coveralls.io/builds/12543159)
Coverage decreased (-0.5%) to 38.673% when pulling **44e132ba77b284e520b5a538f0cffa78f1aa5ec3 on Ritabrata-TW:master** into **d4149207e27ed3516f1f06407b55986790b806ae 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 #134: Rito - #44 - Extracting out duplicated...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r130595893
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
+ } else {
+ // TODO LOG
--- End diff --
@shroman The TODO was already there. Not sure what to do about it right now.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW thanks, I will take a close look it later :-)
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW In other words, your pull request description has to be in this format
```
[ROCKETMQ-xxx] Message describing the issue.
Link to ROCKETMQ-xxx, which is the JIRA issue.
```
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW Could you modify PR subject conforming to our norm :-)
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW Hi, thanks for your contribution, and could you please set your merge target to `develop` branch which is our daily development 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 pull request #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r130764799
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
+ } else {
+ // TODO LOG
--- End diff --
ok then
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@vongosling We do not need a messaging queue in our Project as of yet. Will be advocating for RocketMQ if the scenario arises.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@zhouxinyu Thanks. I will keep the suggested things in mind next time. Please let me know if you need anything to close the PR.
I would also like to get more involved with the RocketMQ community and would be dropping a mentoring request in the mailing list. 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 #134: Rito - #44 - Extracting out duplicated...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r129799560
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -16,29 +16,7 @@
*/
package org.apache.rocketmq.store;
-import java.io.File;
-import java.io.IOException;
-import java.net.SocketAddress;
-import java.nio.ByteBuffer;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedList;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
-import org.apache.rocketmq.common.BrokerConfig;
-import org.apache.rocketmq.common.MixAll;
-import org.apache.rocketmq.common.ServiceThread;
-import org.apache.rocketmq.common.SystemClock;
-import org.apache.rocketmq.common.ThreadFactoryImpl;
-import org.apache.rocketmq.common.UtilAll;
+import org.apache.rocketmq.common.*;
--- End diff --
@shroman Done. Thanks for pointing towards the code guidelines.
---
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 #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r130238297
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
--- End diff --
why do you need 2 breaks?
---
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 #134: Rito - #44 - Extracting out duplicated...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/134#discussion_r130595647
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
--- End diff --
@shroman We don't have two breaks. The part you are have referred to shows the diff where the break from 1220 has been removed and the break on 1215 has been added.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
Thanks @Ritabrata-TW , we will merge this PR soon, but please don't forget to add related unit tests and follow our [code style](http://rocketmq.incubator.apache.org/docs/code-guidelines/) next time.
You are welcome to continue keeping your attention on RocketMQ community.
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@zhouxinyu My pleasure. I have set the merge target to develop. 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 issue #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW this PR has been merged into `develop` branch and will be released in next version, you can close it now safely.
---
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 #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r130764893
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -1215,9 +1210,13 @@ public TopicRouteData getTopicRouteInfoFromNameServer(final String topic, final
assert response != null;
switch (response.getCode()) {
case ResponseCode.TOPIC_NOT_EXIST: {
- if (!topic.equals(MixAll.DEFAULT_TOPIC))
+ if (allowTopicNotExist && !topic.equals(MixAll.DEFAULT_TOPIC)) {
log.warn("get Topic [{}] RouteInfoFromNameServer is not exist value", topic);
- break;
+ break;
--- End diff --
well, I mean you can have one for `case ResponseCode.TOPIC_NOT_EXIST`, don't you?
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
Hi @vsair . Thanks for the suggestions. I have incorporated the same. Please have a look. 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 issue #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by Ritabrata-TW <gi...@git.apache.org>.
Github user Ritabrata-TW commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@vsair The Travis build seems to have passed. Did you notice something wrong with the code style ?
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
[](https://coveralls.io/builds/12574298)
Coverage decreased (-0.3%) to 38.793% when pulling **35194b0136cb178a4e3549dfa39c6c6c0479fbbf on Ritabrata-TW:master** into **d4149207e27ed3516f1f06407b55986790b806ae 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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW I found so many code format in your PR, could you import our code style file as instruction, http://rocketmq.incubator.apache.org/docs/code-guidelines/
---
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 #134: Rito - #44 - Extracting out duplicated code f...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW I am not understand your refer "Story number 44 from the Kanban Board", could you detail 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 pull request #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r129468820
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -745,17 +751,7 @@ public long getMessageStoreTimeStamp(String topic, int queueId, long consumeQueu
ConsumeQueue logicQueue = this.findConsumeQueue(topic, queueId);
if (logicQueue != null) {
SelectMappedBufferResult result = logicQueue.getIndexBuffer(consumeQueueOffset);
- if (result != null) {
- try {
- final long phyOffset = result.getByteBuffer().getLong();
- final int size = result.getByteBuffer().getInt();
- long storeTime = this.getCommitLog().pickupStoreTimestamp(phyOffset, size);
- return storeTime;
- } catch (Exception ignored) {
- } finally {
- result.release();
- }
- }
+ long storeTime = getStoreTime(result);
--- End diff --
Convert Long to long ? may lead to NPE?
---
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 #134: Rito - #44 - Extracting out duplicated...
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/134#discussion_r129781791
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -16,29 +16,7 @@
*/
package org.apache.rocketmq.store;
-import java.io.File;
-import java.io.IOException;
-import java.net.SocketAddress;
-import java.nio.ByteBuffer;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedList;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicLong;
-import org.apache.rocketmq.common.BrokerConfig;
-import org.apache.rocketmq.common.MixAll;
-import org.apache.rocketmq.common.ServiceThread;
-import org.apache.rocketmq.common.SystemClock;
-import org.apache.rocketmq.common.ThreadFactoryImpl;
-import org.apache.rocketmq.common.UtilAll;
+import org.apache.rocketmq.common.*;
--- End diff --
I believe you won't have this * if you format promerly.
Please check http://rocketmq.apache.org/docs/code-guidelines/
---
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 #134: [ROCKETMQ-44] Duplicated Codes in DefaultMess...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/134
@Ritabrata-TW Thanks for your attention for rocketmq community, whats' your scenario when using apache rocketmq in your company :-)
---
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.
---