[GitHub] incubator-rocketmq issue #20: [ROCKETMQ-23] MappedFileQueue#flush should ret...

2017-09-22 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/20 And why is it closed? ---

[GitHub] incubator-rocketmq pull request #133: [ROCKETMQ-249] Do not attempt to clear...

2017-09-22 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/133#discussion_r140425575 --- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java --- @@ -1509,65 +1514,62 @@ private boolean isTimeToDelete

[GitHub] incubator-rocketmq pull request #152: [ROCKETMQ-278] Add clusterlist cmd by ...

2017-09-15 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/152#discussion_r139078626 --- Diff: namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java --- @@ -63,11 +65,36 @@ public RouteInfoManager

[GitHub] incubator-rocketmq issue #154: file test error when make link

2017-09-03 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/154 The title should be "[Rocketmq-285] file test error when make link" --- If your project is set up for it, you can reply to this email and have your reply appear on GitH

[GitHub] incubator-rocketmq pull request #145: [ROCKETMQ-264]Fix unit test cost too l...

2017-08-24 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/145#discussion_r134962143 --- Diff: client/src/test/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java --- @@ -92,9 +92,9

[GitHub] incubator-rocketmq pull request #145: [ROCKETMQ-264]Fix unit test cost too l...

2017-08-17 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/145#discussion_r133863841 --- Diff: client/src/test/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java --- @@ -92,9 +92,9

[GitHub] incubator-rocketmq pull request #145: [ROCKETMQ-264]Fix unit test cost too l...

2017-08-17 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/145#discussion_r133863133 --- Diff: namesrv/src/test/java/org/apache/rocketmq/namesrv/NamesrvControllerTest.java --- @@ -1,46 +0,0 @@ -/* --- End diff

[GitHub] incubator-rocketmq pull request #145: [ROCKETMQ-264]Fix unit test cost too l...

2017-08-17 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/145#discussion_r133862502 --- Diff: broker/src/test/java/org/apache/rocketmq/broker/BrokerControllerTest.java --- @@ -37,16 +37,14 @@ */ @Test

[GitHub] incubator-rocketmq issue #148: [ROCKETMQ-269] don’t need cal.setTimeInMill...

2017-08-17 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/148 I agree with @mark800. IMO, this fix doesn't require unit tests. --- 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

[GitHub] incubator-rocketmq pull request #144: [ROCKETMQ-257] name server address and...

2017-08-11 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/144#discussion_r132808594 --- Diff: common/src/main/java/org/apache/rocketmq/common/MixAll.java --- @@ -59,7 +59,7 @@ public static final String WS_DOMAIN_NAME

[GitHub] incubator-rocketmq pull request #141: [ROCKETMQ-254] fix logger appender uni...

2017-08-08 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/141#discussion_r132088233 --- Diff: logappender/src/main/java/org/apache/rocketmq/logappender/common/ProducerInstance.java --- @@ -70,24 +76,28 @@ public static

[GitHub] incubator-rocketmq pull request #140: [ROCKETMQ-253]Compress RegisterBrokerB...

2017-08-08 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/140#discussion_r132087129 --- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/body/RegisterBrokerBody.java --- @@ -40,4 +57,114 @@ public void

[GitHub] incubator-rocketmq pull request #140: [ROCKETMQ-253]Compress RegisterBrokerB...

2017-08-08 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/140#discussion_r132086276 --- Diff: broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java --- @@ -148,12 +148,13 @@ private RegisterBrokerResult

[GitHub] incubator-rocketmq issue #133: [ROCKETMQ-249] Do not attempt to clear disk e...

2017-08-04 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/133 @zhouxinyu The problem is described in the JIRA issue -- "If disk usage estimates fail because the disk store path is not found (doesn't exist), message store falsely decides the

[GitHub] incubator-rocketmq issue #133: [ROCKETMQ-249] Do not attempt to clear disk e...

2017-08-02 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/133 @vongosling @zhouxinyu @lizhanhui @vsair Anyone's willing to review the PR? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] incubator-rocketmq pull request #139: [ROCKETMQ-242] mqclient can not fetch ...

2017-08-02 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/139#discussion_r131031474 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/factory/MQClientInstance.java --- @@ -255,19 +255,17 @@ public void start

[GitHub] incubator-rocketmq pull request #139: [ROCKETMQ-242] mqclient can not fetch ...

2017-08-02 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/139#discussion_r131030593 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/factory/MQClientInstance.java --- @@ -255,19 +255,17 @@ public void start

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-08-01 Thread shroman
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

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-08-01 Thread shroman
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

[GitHub] incubator-rocketmq issue #127: [ROCKETMQ-233] Fix pull interval issue

2017-08-01 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/127 @lizhanhui Got it, thanks! How about adding comments with explanations for each policy class? I think developers will appreciate that. --- If your project is set up for it, you can

[GitHub] incubator-rocketmq pull request #127: [ROCKETMQ-233] Fix pull interval issue

2017-08-01 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/127#discussion_r130530039 --- Diff: client/src/main/java/org/apache/rocketmq/client/consumer/PullIntervalPolicy.java --- @@ -0,0 +1,25 @@ +/* + * Licensed

[GitHub] incubator-rocketmq pull request #139: [ROCKETMQ-242] mqclient can not fetch ...

2017-08-01 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/139#discussion_r130528221 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/factory/MQClientInstance.java --- @@ -255,19 +255,17 @@ public void start

[GitHub] incubator-rocketmq issue #134: Rito - #44 - Extracting out duplicated code f...

2017-08-01 Thread shroman
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

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-07-30 Thread shroman
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

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-07-30 Thread shroman
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

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-07-30 Thread shroman
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

[GitHub] incubator-rocketmq issue #138: add implement SelectMessageQueueByConsistentH...

2017-07-28 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 `wanting to send a message with...` yes, this should be the top comment of your class ```java /** wanting to send a message with ... */ public class

[GitHub] incubator-rocketmq issue #138: add implement SelectMessageQueueByConsistentH...

2017-07-28 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 Please see this pull request description https://github.com/apache/incubator-rocketmq/pull/130, for example. --- If your project is set up for it, you can reply to this email and have

[GitHub] incubator-rocketmq issue #138: add implement SelectMessageQueueByConsistentH...

2017-07-28 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/138 Also you'll need a JIRA issue opened for this, and specified as the description of this pull request. Please see other accepted pull requests. --- If your project is set up

[GitHub] incubator-rocketmq pull request #134: Rito - #44 - Extracting out duplicated...

2017-07-27 Thread shroman
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

[GitHub] incubator-rocketmq issue #131: [ROCKETMQ-244] Message#putUserProperty uses =...

2017-07-13 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/131 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

[GitHub] incubator-rocketmq pull request #130: [ROCKETMQ-243] BrokerData#selectBroker...

2017-07-12 Thread shroman
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

[GitHub] incubator-rocketmq issue #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...

2017-07-12 Thread shroman
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

[GitHub] incubator-rocketmq pull request #130: [ROCKETMQ-243] BrokerData#selectBroker...

2017-07-11 Thread shroman
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

[GitHub] incubator-rocketmq pull request #130: [ROCKETMQ-243] BrokerData#selectBroker...

2017-07-11 Thread shroman
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

[GitHub] incubator-rocketmq issue #131: [ROCKETMQ-244] Message#putUserProperty uses =...

2017-07-11 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/131 Thanks @Jaskey , updated. --- 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

[GitHub] incubator-rocketmq issue #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...

2017-07-10 Thread shroman
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

[GitHub] incubator-rocketmq issue #130: [ROCKETMQ-243] BrokerData#selectBrokerAddr() ...

2017-07-10 Thread shroman
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

[GitHub] incubator-rocketmq issue #127: [ROCKETMQ-233] Fix pull interval issue

2017-07-10 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/127 Looks good, but can you please briefly outline what the problem was -- ROCKETMQ-233 does not say much. --- If your project is set up for it, you can reply to this email and have your

[GitHub] incubator-rocketmq issue #3: [ROCKETMQ-6] Use logger for exceptions instead ...

2017-07-05 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/3 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

[GitHub] incubator-rocketmq pull request #3: [ROCKETMQ-6] Use logger for exceptions i...

2017-07-05 Thread shroman
Github user shroman closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/3 --- 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

[GitHub] incubator-rocketmq issue #3: [ROCKETMQ-6] Use logger for exceptions instead ...

2017-07-04 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/3 Ok, if it's good to merge, I will resolve the conflicts and 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

[GitHub] incubator-rocketmq issue #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC

2017-06-28 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/119 Another thing is -- do we need the default topic at all? --- 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

[GitHub] incubator-rocketmq issue #106: [ROCKETMQ-202] Using the native transport

2017-05-18 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/106 @lizhanhui Looks good. Can we have some tests? --- 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

[GitHub] incubator-rocketmq issue #60: [ROCKETMQ-96]Rename some temp variable and fie...

2017-05-15 Thread shroman
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

[GitHub] incubator-rocketmq pull request #66: [ROCKETMQ-106] Add flow control on topi...

2017-05-10 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/66#discussion_r115727645 --- Diff: client/src/main/java/org/apache/rocketmq/client/consumer/DefaultMQPushConsumer.java --- @@ -401,6 +408,14 @@ public void

[GitHub] incubator-rocketmq pull request #89: [ROCKETMQ-166] onException callback may...

2017-05-09 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/89#discussion_r115650526 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java --- @@ -306,6 +310,19 @@ public SendResult sendMessage

[GitHub] incubator-rocketmq pull request #80: [ROCKETMQ-114] Add javadoc to codebase

2017-05-04 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733797 --- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/SendMessageProcessor.java --- @@ -112,18 +156,21 @@ private RemotingCommand

[GitHub] incubator-rocketmq pull request #80: [ROCKETMQ-114] Add javadoc to codebase

2017-05-04 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733685 --- Diff: broker/src/main/java/org/apache/rocketmq/broker/BrokerStartup.java --- @@ -95,52 +141,65 @@ public static BrokerController

[GitHub] incubator-rocketmq issue #87: [ROCKETMQ-161] Update runbroker.sh and runserv...

2017-05-04 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/87 @dongeforever Hmm, sorry I don't understand yet why `JAVA_OPTS` won't work. Can you please explain how/when it happens? :) --- If your project is set up for it, you can reply

[GitHub] incubator-rocketmq issue #91: [ROCKETMQ-174]Fix spelling errors

2017-05-04 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/91 Guys, let's merge this fix after the conflicts are resolved. I also don't like breaks in minor versions, but there are too many spelling mistakes and writing workarounds for each

[GitHub] incubator-rocketmq-externals pull request #4: [ROCKETMQ-81] Add the RocketMq...

2017-04-24 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq-externals/pull/4#discussion_r112873634 --- Diff: rocketmq-storm/README.md --- @@ -0,0 +1,2 @@ +# RocketMQ Storm Integration --- End diff -- Is this pushed

[GitHub] incubator-rocketmq issue #86: [ROCKETMQ-160]SendHeartBeat may not be logged ...

2017-04-19 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/86 @Jaskey Incrementing and logging heartbeats on success seems reasonable to me. All failures are logged in `catch` clause. --- If your project is set up for it, you can reply

[GitHub] incubator-rocketmq issue #87: [ROCKETMQ-161] Update runbroker.sh and runserv...

2017-04-18 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/87 Ok, but let's set it through `JVM_OPTS` instead of introducing a variable for that specific setting. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] incubator-rocketmq issue #87: [ROCKETMQ-161] Update runbroker.sh and runserv...

2017-04-16 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/87 @dongeforever Sorry, I don't think this modification is needed. A user will modify this file anyway in order to match his/her system requirements. Also, there are even more flags

[GitHub] incubator-rocketmq pull request #91: [ROCKETMQ-174]Fix spelling errors

2017-04-16 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/91#discussion_r111704133 --- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/header/GetEarliestMsgStoreTimeResponseHeader.java --- @@ -16,15 +16,15

[GitHub] incubator-rocketmq pull request #91: [ROCKETMQ-174]Fix spelling errors

2017-04-16 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/91#discussion_r111704125 --- Diff: common/src/main/java/org/apache/rocketmq/common/protocol/header/GetEarliestMsgStoreTimeRequestHeader.java --- @@ -16,15 +16,15

[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...

2017-04-16 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111700022 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java --- @@ -577,12 +577,12 @@ public void operationComplete

[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...

2017-04-16 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111699977 --- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java --- @@ -577,12 +577,12 @@ public void operationComplete

[GitHub] incubator-rocketmq-externals issue #5: [ROCKETMQ-81] import rocketmq plugin ...

2017-03-30 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq-externals/pull/5 @zhouxinyu yes, luckily they are :) IMO, if the difference is just PUSH and PULL models, it's reasonable to merge them and improve. Any other ideas? --- If your project

[GitHub] incubator-rocketmq-externals issue #5: [ROCKETMQ-81] import rocketmq plugin ...

2017-03-30 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq-externals/pull/5 @vongosling There are two PRs for ROCKETMQ-81. I guess @hustfxj or @vesense will have to compile them in one, and they recheck it together :) Guys, please make sure you

[GitHub] incubator-rocketmq pull request #56: [ROCKETMQ-90] Include client IP in cons...

2017-03-29 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/56#discussion_r108617467 --- Diff: tools/src/main/java/org/apache/rocketmq/tools/command/consumer/ConsumerProgressSubCommand.java --- @@ -94,12 +117,14 @@ public void

[GitHub] incubator-rocketmq pull request #52: [ROCKETMQ-76] Expose IntegrationTestBas...

2017-03-29 Thread shroman
Github user shroman closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/52 --- 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

[GitHub] incubator-rocketmq issue #52: [ROCKETMQ-76] Expose IntegrationTestBase to be...

2017-03-29 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/52 @dongeforever Yes, sure. But first please address my comments for that PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] incubator-rocketmq issue #52: [ROCKETMQ-76] Expose IntegrationTestBase to be...

2017-03-29 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/52 @dongeforever Thanks! It seems there are no objections, I will merge it to `develop` branch. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] incubator-rocketmq issue #83: [ROCKETMQ-154] add a newline after help info

2017-03-27 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/83 Merged to `develop` branch. Thanks @vesense. Please close 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

[GitHub] incubator-rocketmq issue #75: [ROCKETMQ-138]Add AuthenticationException clas...

2017-03-27 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 If no special consideration for authentication errors is needed (which was obviously the intention in the original code), +1 for just removing the hard-coded snippet

[GitHub] incubator-rocketmq issue #84: [ROCKETMQ-155] fix typo in ClientConfig

2017-03-27 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/84 +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

[GitHub] incubator-rocketmq-site pull request #8: Added having unit tests as a necess...

2017-03-16 Thread shroman
Github user shroman closed the pull request at: https://github.com/apache/incubator-rocketmq-site/pull/8 --- 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

[GitHub] incubator-rocketmq-site issue #8: Added having unit tests as a necessary con...

2017-03-15 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq-site/pull/8 @zhouxinyu Sure, I just wanted to be sure this addition to the PR submission policy is ok ;) --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] incubator-rocketmq pull request #60: [ROCKETMQ-96]Rename some temp variable ...

2017-03-12 Thread shroman
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

[GitHub] incubator-rocketmq pull request #60: [ROCKETMQ-96]Rename some temp variable ...

2017-03-12 Thread shroman
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

[GitHub] incubator-rocketmq issue #73: [ROCKETMQ-135] Broker cannot be properly final...

2017-03-12 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/73 @vongosling @zhouxinyu @lizhanhui --- 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

[GitHub] incubator-rocketmq issue #76: Release 4.0.0 incubating

2017-03-12 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/76 @eoss1990 Have you created this PR by mistake? :) --- 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

[GitHub] incubator-rocketmq issue #73: [ROCKETMQ-135] Broker cannot be properly final...

2017-03-12 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/73 I would rather get suggestions about this modification, if any, instead of hypothetical considerations. What is described here cannot be done with the current RocketMQ code base -- it's

[GitHub] incubator-rocketmq issue #73: [ROCKETMQ-135] Broker cannot be properly final...

2017-03-10 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/73 @Jaskey For this, write your decorated code (what you write above) in, say, `MyDecoratedClass` and specify "MyDecoratedClass" as your storage to initialize. That will suffice.

[GitHub] incubator-rocketmq issue #73: [ROCKETMQ-135] Broker cannot be properly final...

2017-03-10 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/73 @Jaskey For a decorated class, you don't write "DecoratedClassA, DecoratedClassB, DecoratedClassC" in your properties, do you? :) You just specify your outer class, say "

[GitHub] incubator-rocketmq issue #75: Add AuthenticationException class to remove ha...

2017-03-10 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/75 :+1: to this fix. --- 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

[GitHub] incubator-rocketmq issue #51: [ROCKETMQ-75] RemotingCommand header decoding ...

2017-03-09 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/51 @lizhanhui @vongosling I see. I changed the code, please have a look. Also, I will try to keep the modules in mind, but we need something not to forget they have to be treated

[GitHub] incubator-rocketmq issue #51: [ROCKETMQ-75] RemotingCommand header decoding ...

2017-03-09 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/51 @vongosling Is it for `rocketmq-remoting` only or for the whole project? It doesn't sound exciting to go back to old JDKs, but ok :) --- If your project is set up for it, you can

[GitHub] incubator-rocketmq issue #67: [ROCKETMQ-67] Consistent Hash allocate strateg...

2017-03-09 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/67 @Jaskey This is a very good description, and now reviewers will understand well your intentions :) If you clearly state your intentions in JIRA, or discuss in the ml, it saves much time

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-03-08 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Jaskey I agree that `setServiceState` should be deprecated. I overlooked it was _public_. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] incubator-rocketmq issue #69: [ROCKETMQ-111] fix possible MQClientException ...

2017-03-07 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/69 @vongosling I meant "If I were you, I would ..." :) Anyway, I don't see why this pr can be merged if a unit test can be easily written. I think we have to accept PRs with tes

[GitHub] incubator-rocketmq-site pull request #8: Added having unit tests as a necess...

2017-03-07 Thread shroman
GitHub user shroman opened a pull request: https://github.com/apache/incubator-rocketmq-site/pull/8 Added having unit tests as a necessary condition for PR (when applica… …ble). You can merge this pull request into a Git repository by running: $ git pull https://github.com

[GitHub] incubator-rocketmq issue #69: [ROCKETMQ-111] fix possible MQClientException ...

2017-03-07 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/69 @zhouxinyu Probably you misunderstood what I meant :) -- this PR needs unit tests. I think it's the responsibility of the person who submitted PR to create them, whenever possible, since

[GitHub] incubator-rocketmq issue #69: [ROCKETMQ-111] fix possible MQClientException ...

2017-02-27 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/69 Good catch. I would add unit tests though ;) --- 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

[GitHub] incubator-rocketmq issue #65: [ROCKETMQ-104] Make MQAdmin commands throw exc...

2017-02-26 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/65 @vongosling Thanks for reviewing! Sure, unit tests can be completed after we improve this module. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 I also think that using `synchronized` fits well here. But I would do it in the following way: 1. Make `serviceState` volatile ```java private volatile ServiceState

[GitHub] incubator-rocketmq issue #65: [ROCKETMQ-104] Make MQAdmin commands throw exc...

2017-02-17 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/65 @zhouxinyu Thanks for checking it! Well, `@Ignore` is there just for finer granularity -- we remove it one by one when implementing each test. If you think we don't need it, I can

[GitHub] incubator-rocketmq pull request #61: [ROCKETMQ-98]Fix risk of unable to rele...

2017-02-16 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/61#discussion_r101699903 --- Diff: common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java --- @@ -47,6 +47,7 @@ private boolean

[GitHub] incubator-rocketmq pull request #65: [ROCKETMQ-104] Make MQAdmin commands th...

2017-02-16 Thread shroman
GitHub user shroman opened a pull request: https://github.com/apache/incubator-rocketmq/pull/65 [ROCKETMQ-104] Make MQAdmin commands throw exceptions JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-104 You can merge this pull request into a Git repository by running

[GitHub] incubator-rocketmq pull request #57: [ROCKETMQ-91] Reduce lock granularity f...

2017-02-15 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/57#discussion_r101456741 --- Diff: store/src/main/java/org/apache/rocketmq/store/CommitLog.java --- @@ -1154,7 +1249,7 @@ public AppendMessageResult doAppend(final long

[GitHub] incubator-rocketmq pull request #57: [ROCKETMQ-91] Reduce lock granularity f...

2017-02-15 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/57#discussion_r101455998 --- Diff: store/src/main/java/org/apache/rocketmq/store/CommitLog.java --- @@ -567,6 +577,11 @@ public PutMessageResult putMessage(final

[GitHub] incubator-rocketmq-site pull request #7: Changes to the text on specifying n...

2017-02-10 Thread shroman
Github user shroman closed the pull request at: https://github.com/apache/incubator-rocketmq-site/pull/7 --- 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

[GitHub] incubator-rocketmq issue #53: [ROCKETMQ-80] Add batch feature

2017-02-07 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/53 Sounds good to check for the protocol version in the request header, and reject with an error when not matched. Probably, `RemotingCommand.REMOTING_VERSION_KEY` will work, but I would

[GitHub] incubator-rocketmq pull request #49: [ROCKETMQ-72] DefaultMessageStore canno...

2017-01-23 Thread shroman
GitHub user shroman opened a pull request: https://github.com/apache/incubator-rocketmq/pull/49 [ROCKETMQ-72] DefaultMessageStore cannot be properly shutdown when fa… …iled in the middle of start() JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-72

[GitHub] incubator-rocketmq issue #37: [ROCKETMQ-38] Some unit tests for rocketmq-rem...

2017-01-13 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/37 A suggestion: how about renaming this JIRA issue? This issue is for RemotingCommand, _'some'_ is very vague. We can end up with many more _'some'_ tests in future :) --- If your

[GitHub] incubator-rocketmq-site pull request #4: Improved 'How to create a PR (contr...

2017-01-12 Thread shroman
Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq-site/pull/4#discussion_r95923375 --- Diff: _docs/06-best-practice-pull-request.md --- @@ -73,11 +73,15 @@ Push your branch to Github: 6. When you are satisfied and want

[GitHub] incubator-rocketmq issue #3: [ROCKETMQ-6] Use logger for exceptions instead ...

2017-01-09 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/3 Hi @zhouxinyu I am not sure if I understand your 1st point though, but if you are saying about printing to `stdout` or `stderr` so that users can find problems with brokers from

[GitHub] incubator-rocketmq issue #29: [ROCKETMQ-31] Fix "No such file or directory" ...

2017-01-08 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/29 @zhouxinyu @stevenschew Thanks, guys, https://issues.apache.org/jira/browse/ROCKETMQ-36 is created to consider the improvement. --- If your project is set up for it, you can reply

  1   2   >