You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by mark800 <gi...@git.apache.org> on 2017/08/14 13:47:52 UTC
[GitHub] incubator-rocketmq pull request #147: Can���t start consumer with a small co...
GitHub user mark800 opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/147
Can’t start consumer with a small consumerThreadMax number
Can’t start consumer with a small consumerThreadMax number
--
When the client set the consumerThreadMax to a value less than the default value(20),
we get the “consumeThreadMin Out of range [1, 1000] “ Exception, which is hard to understand.
JIRA Issue: https://issues.apache.org/jira/browse/ROCKETMQ-266
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mark800/incubator-rocketmq master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/147.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 #147
----
commit ba687075cfe14b25ee2c8e36f9da8452ba0734f0
Author: mark.yky <ma...@alibaba-inc.com>
Date: 2017-08-14T13:41:12Z
Can’t start consumer with a small consumerThreadMax number
----
---
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 #147: [ROCKETMQ-266] Add a specific Exceptio...
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/147#discussion_r133349431
--- Diff: client/src/test/java/org/apache/rocketmq/client/impl/consumer/DefaultMQPushConsumerImplTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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.client.impl.consumer;
+
+import java.util.List;
+import org.apache.rocketmq.client.consumer.DefaultMQPushConsumer;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyContext;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyStatus;
+import org.apache.rocketmq.client.consumer.listener.MessageListenerConcurrently;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.junit.Test;
+
+public class DefaultMQPushConsumerImplTest {
+ @Test(expected = MQClientException.class)
+ public void checkConfigTest() throws Exception {
+ DefaultMQPushConsumer consumer = new DefaultMQPushConsumer("test_consumer_group");
+
+ consumer.setConsumeThreadMax(10);
--- End diff --
In the unit test , we should specify the min explicitly as well
---
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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12919598)
Coverage increased (+0.05%) to 39.119% when pulling **1590ed252af50283ff402edd448076fffa95eb5b on mark800:master** into **332df78e2bb6caadfc6f4714450000ebb1f9d224 on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #147: [ROCKETMQ-266] Add a specific Exceptio...
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/147#discussion_r133633277
--- Diff: client/src/test/java/org/apache/rocketmq/client/impl/consumer/DefaultMQPushConsumerImplTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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.client.impl.consumer;
+
+import java.util.List;
+import org.apache.rocketmq.client.consumer.DefaultMQPushConsumer;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyContext;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyStatus;
+import org.apache.rocketmq.client.consumer.listener.MessageListenerConcurrently;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.junit.Test;
+
+public class DefaultMQPushConsumerImplTest {
+ @Test(expected = MQClientException.class)
+ public void checkConfigTest() throws Exception {
+ DefaultMQPushConsumer consumer = new DefaultMQPushConsumer("test_consumer_group");
+
+ consumer.setConsumeThreadMax(10);
--- End diff --
@mark800 , I think we should make it more explict since the default max core size may be changed in the future, make it explicit will be clearer.
If you unit test, you are only verifying an expcetion is thrown which is not enough to verify this pr since an exception has been thrown even though you don't submit the changes, you should also very the error message is as expected.
---
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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12879904)
Coverage increased (+0.07%) to 39.137% when pulling **074e457a1c158b758079d5ca4197dcfa0fc9430b on mark800:master** into **332df78e2bb6caadfc6f4714450000ebb1f9d224 on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by mark800 <gi...@git.apache.org>.
Github user mark800 commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
The travis-ci's error log is :
$ mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
Error: JAVA_HOME is not defined correctly.
We cannot execute /usr/lib/jvm/java-7-oracle/bin/java
The command "eval mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V " failed.
It seems it is not the code's problem
---
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 #147: Can���t start consumer with a small consumerT...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12836052)
Coverage increased (+0.003%) to 38.785% when pulling **bf451ec3dfacbf66d0c32f3145b60395d1e2db8f on mark800:master** into **421a22c73bfb5d1c4748f7d23b0a2a4fd5586192 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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12888375)
Coverage increased (+0.007%) to 39.078% when pulling **5f5ee6d2089a146f8a3437ae939c28985b38aaaf on mark800:master** into **332df78e2bb6caadfc6f4714450000ebb1f9d224 on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
While consumeThreadMin is specified by a range,why not add consumeThreadMax to a range too?
---
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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by mark800 <gi...@git.apache.org>.
Github user mark800 commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
@lindzh
My opinion is：consumeThreadMax and consumeThreadMin are two separate configuration,
if we only configure one parameter, it should work, or at least give a reasonable exception message.
If the two configurations have to be used at the same time, we should combine them to one configure option.
---
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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
LGTM @zhouxinyu @vongosling any view?
---
[GitHub] incubator-rocketmq issue #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12876682)
Coverage decreased (-0.004%) to 39.066% when pulling **a09bc6c1291a3938bfaeb4448666b8ef3204f71e on mark800:master** into **332df78e2bb6caadfc6f4714450000ebb1f9d224 on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #147: Can���t start consumer with a small consumerT...
Posted by vsair <gi...@git.apache.org>.
Github user vsair commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
Thanks for your contributions. But,
1. Please refer to http://rocketmq.apache.org/docs/pull-request/, then modify your pr title.
2. Please change the merge target to develop.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/13183888)
Coverage increased (+0.03%) to 38.743% when pulling **7d2f899e15d7ed4127657a49034bdae892e4174b on mark800:master** into **845830865fc37d0364a19cbd89ceaf8a30b37e1c on apache:develop**.
---
[GitHub] incubator-rocketmq pull request #147: [ROCKETMQ-266] Add a specific Exceptio...
Posted by mark800 <gi...@git.apache.org>.
Github user mark800 commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/147#discussion_r133365826
--- Diff: client/src/test/java/org/apache/rocketmq/client/impl/consumer/DefaultMQPushConsumerImplTest.java ---
@@ -0,0 +1,48 @@
+/*
+ * 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.client.impl.consumer;
+
+import java.util.List;
+import org.apache.rocketmq.client.consumer.DefaultMQPushConsumer;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyContext;
+import org.apache.rocketmq.client.consumer.listener.ConsumeConcurrentlyStatus;
+import org.apache.rocketmq.client.consumer.listener.MessageListenerConcurrently;
+import org.apache.rocketmq.client.exception.MQClientException;
+import org.apache.rocketmq.common.message.MessageExt;
+import org.junit.Test;
+
+public class DefaultMQPushConsumerImplTest {
+ @Test(expected = MQClientException.class)
+ public void checkConfigTest() throws Exception {
+ DefaultMQPushConsumer consumer = new DefaultMQPushConsumer("test_consumer_group");
+
+ consumer.setConsumeThreadMax(10);
--- End diff --
The test case’s purpose is: If the consumeThreadMax is less than consumeThreadMin, we should get a proper exception message, the one line is enough.
The DefaultMQPushConsumerImpl.java class needs a lot more test case, a lot of condition should be checked, but that’s not this PR’s purpose.
---
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 #147: Add a specific Exception message for comparin...
Posted by mark800 <gi...@git.apache.org>.
Github user mark800 commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
@vsair
Thanks for your advice!
---
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 #147: Can���t start consumer with a small consumerT...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
[](https://coveralls.io/builds/12823734)
Coverage decreased (-0.3%) to 38.465% when pulling **6288e5696aa8287e20cb23fa95f2071c2a51c85f on mark800:master** into **421a22c73bfb5d1c4748f7d23b0a2a4fd5586192 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 #147: [ROCKETMQ-266] Add a specific Exception messa...
Posted by mark800 <gi...@git.apache.org>.
Github user mark800 commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/147
@Jaskey
Thanks for your advice, I've updated the code.
---
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.
---