You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by lindzh <gi...@git.apache.org> on 2017/06/23 11:01:56 UTC
[GitHub] incubator-rocketmq pull request #126: [ROCKETMQ-231] fix pull consumer pull ...
GitHub user lindzh opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/126
[ROCKETMQ-231] fix pull consumer pull result size
When using PullConsumer pull message by default result size is 32,and messages is more than 32 in a queue,but broker always returns 31.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lindzh/incubator-rocketmq fix_consumer_pull_msg_size
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/126.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 #126
----
commit 1d27251df0265e19397cffc1ee5098e12bc4cd0d
Author: lindzh <li...@163.com>
Date: 2017-06-23T10:56:49Z
fix pull consumer pull result size
----
---
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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/126#discussion_r124758258
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
+
+ private static final Map<MessageQueue, Long> OFFSE_TABLE = new HashMap<MessageQueue, Long>();
+
+ public static final String PULL_SIZE_TOPIC = "TopicPullTest";
+
+ public static final String PULL_SIZE_GROUP = "pullSizeTest";
+
+ public boolean send() throws MQClientException, InterruptedException {
+ DefaultMQProducer producer = new DefaultMQProducer(PULL_SIZE_GROUP);
+ producer.setNamesrvAddr(nsAddr);
+ producer.start();
+ int successCount = 0;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ Message msg = new Message(PULL_SIZE_TOPIC, "TagA", ("RocketMQ pull size test index " + i).getBytes(RemotingHelper.DEFAULT_CHARSET));
+ SendResult sendResult = producer.send(msg);
+ if (sendResult.getSendStatus().equals(SendStatus.SEND_OK)) {
+ successCount++;
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ Thread.sleep(500);
+ }
+ }
+ producer.shutdown();
+ if (successCount > 800) {
--- End diff --
This test case has removed and add new unit test in store.
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
Now, 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/126#discussion_r132622123
--- Diff: store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest.java ---
@@ -45,19 +47,22 @@ public void init() throws Exception {
BornHost = new InetSocketAddress(InetAddress.getByName("127.0.0.1"), 0);
}
+ public MessageStore buildMessageStore() throws Exception {
+ MessageStoreConfig messageStoreConfig = new MessageStoreConfig();
+ messageStoreConfig.setMapedFileSizeCommitLog(1024 * 1024 * 10);
+ messageStoreConfig.setMapedFileSizeConsumeQueue(1024 * 1024 * 10);
+ messageStoreConfig.setMaxHashSlotNum(10000);
+ messageStoreConfig.setMaxIndexNum(100 * 100);
+ messageStoreConfig.setFlushDiskType(FlushDiskType.ASYNC_FLUSH);
+ return new DefaultMessageStore(messageStoreConfig, new BrokerStatsManager("simpleTest"), new MyMessageArrivingListener(), new BrokerConfig());
--- End diff --
Indeed this is a must after testing,and this bug has been fix in PR https://github.com/apache/incubator-rocketmq/pull/141
---
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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r125448602
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -1110,7 +1110,7 @@ private boolean isTheBatchFull(int sizePy, int maxMsgNums, int bufferTotal, int
return false;
}
- if ((messageTotal + 1) >= maxMsgNums) {
--- End diff --
I will still suggest using `>=`
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12636404)
Coverage increased (+0.9%) to 39.575% when pulling **a5d73d080e49e63457c2be4b996c339d6e488fae on lindzh:fix_consumer_pull_msg_size** into **9bb6eae4bd35697808174a2ff9195e393450ec91 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/126#discussion_r125196481
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -1110,7 +1110,7 @@ private boolean isTheBatchFull(int sizePy, int maxMsgNums, int bufferTotal, int
return false;
}
- if ((messageTotal + 1) >= maxMsgNums) {
--- End diff --
Yet,That a good idea to make code clean
---
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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r123731300
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
+
+ private static final Map<MessageQueue, Long> OFFSE_TABLE = new HashMap<MessageQueue, Long>();
+
+ public static final String PULL_SIZE_TOPIC = "TopicPullTest";
+
+ public static final String PULL_SIZE_GROUP = "pullSizeTest";
+
+ public boolean send() throws MQClientException, InterruptedException {
+ DefaultMQProducer producer = new DefaultMQProducer(PULL_SIZE_GROUP);
+ producer.setNamesrvAddr(nsAddr);
+ producer.start();
+ int successCount = 0;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ Message msg = new Message(PULL_SIZE_TOPIC, "TagA", ("RocketMQ pull size test index " + i).getBytes(RemotingHelper.DEFAULT_CHARSET));
+ SendResult sendResult = producer.send(msg);
+ if (sendResult.getSendStatus().equals(SendStatus.SEND_OK)) {
+ successCount++;
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ Thread.sleep(500);
+ }
+ }
+ producer.shutdown();
+ if (successCount > 800) {
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+
+ public void pullMsg() throws MQClientException {
+ boolean result = false;
+ DefaultMQPullConsumer consumer = new DefaultMQPullConsumer(PULL_SIZE_GROUP);
+ consumer.setNamesrvAddr(nsAddr);
+ consumer.start();
+ Set<MessageQueue> mqs = consumer.fetchSubscribeMessageQueues(PULL_SIZE_TOPIC);
+ for (MessageQueue mq : mqs) {
+ if (result) {
+ break;
+ }
+ try {
+ PullResult pullResult = consumer.pull(mq, null, getMessageQueueOffset(mq), 32);
+ putMessageQueueOffset(mq, pullResult.getNextBeginOffset());
+ switch (pullResult.getPullStatus()) {
+ case FOUND:
+ List<MessageExt> msgFoundList = pullResult.getMsgFoundList();
+ if (msgFoundList != null) {
+ logger.info("[RECV] received msg queue:" + mq.getBrokerName() + "-" + mq.getQueueId());
+ result |= msgFoundList.size() >= 32;
+ }
+ break;
+ case NO_MATCHED_MSG:
+ break;
+ case NO_NEW_MSG:
+ break;
+ case OFFSET_ILLEGAL:
+ break;
+ default:
+ break;
+ }
+ } catch (Exception e) {
+ logger.error("[SEND] send failed", e);
+ e.printStackTrace();
+ }
+ }
+ consumer.shutdown();
+ Assert.assertTrue(result);
--- End diff --
Could we assert the pull size is what you want, while not the simple true or false ?
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by lollipopjin <gi...@git.apache.org>.
Github user lollipopjin commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
+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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/126#discussion_r132427103
--- Diff: store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest.java ---
@@ -45,19 +47,22 @@ public void init() throws Exception {
BornHost = new InetSocketAddress(InetAddress.getByName("127.0.0.1"), 0);
}
+ public MessageStore buildMessageStore() throws Exception {
+ MessageStoreConfig messageStoreConfig = new MessageStoreConfig();
+ messageStoreConfig.setMapedFileSizeCommitLog(1024 * 1024 * 10);
+ messageStoreConfig.setMapedFileSizeConsumeQueue(1024 * 1024 * 10);
+ messageStoreConfig.setMaxHashSlotNum(10000);
+ messageStoreConfig.setMaxIndexNum(100 * 100);
+ messageStoreConfig.setFlushDiskType(FlushDiskType.ASYNC_FLUSH);
+ return new DefaultMessageStore(messageStoreConfig, new BrokerStatsManager("simpleTest"), new MyMessageArrivingListener(), new BrokerConfig());
--- End diff --
The MessageStore uses the default store path(~/store).
And the test forgets to delete files in that path.
Two suggestions:
1. create temp path, and set it as storePath
2. delete all the files after the testing
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12636404)
Coverage increased (+0.9%) to 39.575% when pulling **a5d73d080e49e63457c2be4b996c339d6e488fae on lindzh:fix_consumer_pull_msg_size** into **9bb6eae4bd35697808174a2ff9195e393450ec91 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r124461959
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
--- End diff --
@zhouxinyu aggree
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12636413)
Coverage increased (+0.6%) to 39.308% when pulling **a5d73d080e49e63457c2be4b996c339d6e488fae on lindzh:fix_consumer_pull_msg_size** into **9bb6eae4bd35697808174a2ff9195e393450ec91 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r123730667
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
--- End diff --
Could we remove logger in unit test ?
---
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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/126#discussion_r130558354
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -1110,7 +1110,7 @@ private boolean isTheBatchFull(int sizePy, int maxMsgNums, int bufferTotal, int
return false;
}
- if ((messageTotal + 1) >= maxMsgNums) {
--- End diff --
Yes,this problem has been fixed to maxMsgNums <= messageTotal
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12228663)
Coverage increased (+0.5%) to 39.59% when pulling **bc492f514052673c9f80ff1b156ab301fabb0983 on lindzh:fix_consumer_pull_msg_size** into **c297258d219277c932a84d37fec19aeaea54588a 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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12185077)
Coverage increased (+0.4%) to 39.467% when pulling **54ce8e31a33fa0f9cefad7acf0d1a9020bde617e on lindzh:fix_consumer_pull_msg_size** into **c297258d219277c932a84d37fec19aeaea54588a 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/126
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://:/builds/12101402)
Coverage decreased (-2.4%) to 36.756% when pulling **1d27251df0265e19397cffc1ee5098e12bc4cd0d on lindzh:fix_consumer_pull_msg_size** into **c297258d219277c932a84d37fec19aeaea54588a 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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12636404)
Coverage increased (+0.9%) to 39.575% when pulling **a5d73d080e49e63457c2be4b996c339d6e488fae on lindzh:fix_consumer_pull_msg_size** into **9bb6eae4bd35697808174a2ff9195e393450ec91 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r123921604
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
--- End diff --
IMO, we don't need a separate it test file to cover this point, use unit test.
---
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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12200598)
Coverage increased (+0.5%) to 39.598% when pulling **a35a4e38e036f35f46705c556949ed98ab607650 on lindzh:fix_consumer_pull_msg_size** into **c297258d219277c932a84d37fec19aeaea54588a 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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
[](https://coveralls.io/builds/12185077)
Coverage increased (+0.4%) to 39.467% when pulling **54ce8e31a33fa0f9cefad7acf0d1a9020bde617e on lindzh:fix_consumer_pull_msg_size** into **c297258d219277c932a84d37fec19aeaea54588a 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r123730992
--- Diff: test/src/test/java/org/apache/rocketmq/test/client/consumer/pull/PullSizeTest.java ---
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.test.client.consumer.pull;
+
+import org.apache.log4j.Logger;
+import org.apache.rocketmq.client.consumer.DefaultMQPullConsumer;
+import org.apache.rocketmq.client.consumer.PullResult;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.client.producer.DefaultMQProducer;
+import org.apache.rocketmq.client.producer.SendResult;
+import org.apache.rocketmq.client.producer.SendStatus;
+import org.apache.rocketmq.common.message.Message;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.apache.rocketmq.common.message.MessageQueue;
+import org.apache.rocketmq.remoting.common.RemotingHelper;
+import org.apache.rocketmq.test.base.BaseConf;
+import org.apache.rocketmq.test.base.IntegrationTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.nio.channels.Pipe;
+import java.util.*;
+
+public class PullSizeTest extends BaseConf {
+
+ private static Logger logger = Logger.getLogger(PullSizeTest.class);
+
+ private static final Map<MessageQueue, Long> OFFSE_TABLE = new HashMap<MessageQueue, Long>();
+
+ public static final String PULL_SIZE_TOPIC = "TopicPullTest";
+
+ public static final String PULL_SIZE_GROUP = "pullSizeTest";
+
+ public boolean send() throws MQClientException, InterruptedException {
+ DefaultMQProducer producer = new DefaultMQProducer(PULL_SIZE_GROUP);
+ producer.setNamesrvAddr(nsAddr);
+ producer.start();
+ int successCount = 0;
+ for (int i = 0; i < 1000; i++) {
+ try {
+ Message msg = new Message(PULL_SIZE_TOPIC, "TagA", ("RocketMQ pull size test index " + i).getBytes(RemotingHelper.DEFAULT_CHARSET));
+ SendResult sendResult = producer.send(msg);
+ if (sendResult.getSendStatus().equals(SendStatus.SEND_OK)) {
+ successCount++;
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ Thread.sleep(500);
+ }
+ }
+ producer.shutdown();
+ if (successCount > 800) {
--- End diff --
why use this magic number 800 in your if statement
---
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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r125432988
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -1110,7 +1110,7 @@ private boolean isTheBatchFull(int sizePy, int maxMsgNums, int bufferTotal, int
return false;
}
- if ((messageTotal + 1) >= maxMsgNums) {
--- End diff --
why change to -1 in your new 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 #126: [ROCKETMQ-231] fix pull consumer pull result ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/126
Thanks @lindzh, i have merged it. 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 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 #126: [ROCKETMQ-231] fix pull consumer pull ...
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/126#discussion_r124975570
--- Diff: store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java ---
@@ -1110,7 +1110,7 @@ private boolean isTheBatchFull(int sizePy, int maxMsgNums, int bufferTotal, int
return false;
}
- if ((messageTotal + 1) >= maxMsgNums) {
--- End diff --
I will suggest replace all the condition of `messageTotal +1 > xxx` into
messageTotal >= xxx
---
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.
---