Skip to content

[ROCKETMQ-266] Add a specific Exception message for comparing consumerThreadMax and consumerThreadMin #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
vongosling merged 13 commits into from
Dec 13, 2017
Merged

[ROCKETMQ-266] Add a specific Exception message for comparing consumerThreadMax and consumerThreadMin #147

vongosling merged 13 commits into from
Dec 13, 2017

Conversation

@mark800
Copy link
Contributor

@mark800 mark800 commented Aug 14, 2017 •
edited
Loading

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

@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-0.3%) to 38.465% when pulling 6288e56 on mark800:master into 421a22c on apache:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-0.3%) to 38.465% when pulling 6288e56 on mark800:master into 421a22c on apache:master.

@vsair
Copy link
Contributor

vsair commented Aug 15, 2017

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.

@coveralls
Copy link

coveralls commented Aug 15, 2017

Coverage Status

Coverage increased (+0.003%) to 38.785% when pulling bf451ec on mark800:master into 421a22c on apache:master.

2 similar comments
@coveralls
Copy link

coveralls commented Aug 15, 2017

Coverage Status

Coverage increased (+0.003%) to 38.785% when pulling bf451ec on mark800:master into 421a22c on apache:master.

@coveralls
Copy link

coveralls commented Aug 15, 2017

Coverage Status

Coverage increased (+0.003%) to 38.785% when pulling bf451ec on mark800:master into 421a22c on apache:master.

@mark800 mark800 changed the title Can’t start consumer with a small consumerThreadMax number Add a specific Exception message for comparing consumerThreadMax and consumerThreadMin Aug 15, 2017
@mark800
Copy link
Contributor Author

mark800 commented Aug 15, 2017

@vsair
Thanks for your advice!

@mark800 mark800 changed the title Add a specific Exception message for comparing consumerThreadMax and consumerThreadMin [ROCKETMQ-266] Add a specific Exception message for comparing consumerThreadMax and consumerThreadMin Aug 15, 2017
@lindzh
Copy link
Contributor

lindzh commented Aug 15, 2017

While consumeThreadMin is specified by a range,why not add consumeThreadMax to a range too?

@mark800
Copy link
Contributor Author

mark800 commented Aug 15, 2017

@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.

Jaskey
Jaskey reviewed Aug 16, 2017
View reviewed changes
...nt/src/test/java/org/apache/rocketmq/client/impl/consumer/DefaultMQPushConsumerImplTest.java Outdated
public void checkConfigTest() throws Exception {
DefaultMQPushConsumer consumer = new DefaultMQPushConsumer("test_consumer_group");

consumer.setConsumeThreadMax(10);
Copy link
Contributor

@Jaskey Jaskey Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the unit test , we should specify the min explicitly as well

Copy link
Contributor Author

@mark800 mark800 Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Jaskey Jaskey Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Besides, in you current 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.

@mark800 mark800 changed the base branch from master to develop August 17, 2017 11:48
@mark800
Copy link
Contributor Author

mark800 commented Aug 17, 2017

@Jaskey
Thanks for your advice, I've updated the code.

@coveralls
Copy link

coveralls commented Aug 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.004%) to 39.066% when pulling a09bc6c on mark800:master into 332df78 on apache:develop.

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage increased (+0.07%) to 39.137% when pulling 074e457 on mark800:master into 332df78 on apache:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage increased (+0.07%) to 39.137% when pulling 074e457 on mark800:master into 332df78 on apache:develop.

@mark800
Copy link
Contributor Author

mark800 commented Aug 18, 2017

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

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage increased (+0.007%) to 39.078% when pulling 5f5ee6d on mark800:master into 332df78 on apache:develop.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage increased (+0.007%) to 39.078% when pulling 5f5ee6d on mark800:master into 332df78 on apache:develop.

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.05%) to 39.119% when pulling 1590ed2 on mark800:master into 332df78 on apache:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.05%) to 39.119% when pulling 1590ed2 on mark800:master into 332df78 on apache:develop.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.03%) to 38.743% when pulling 7d2f899 on mark800:master into 8458308 on apache:develop.

@dongeforever
Copy link
Member

dongeforever commented Sep 21, 2017

LGTM @zhouxinyu @vongosling any view?

@vongosling vongosling merged commit 947526b into apache:develop Dec 13, 2017
@vongosling vongosling added this to the 4.3.0 milestone Jul 17, 2018
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
@YangJodie
Issue apache#147 benchmark 添加指定 topic
98fcaf1
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
@YangJodie
Issue apache#147 benchmark 添加指定 group
0708c0e
lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019
@YangJodie
Issue apache#147 benchmark 添加指定 group
ac9c1c1
renshuaibing-aaron pushed a commit to renshuaibing-aaron/rocketmq that referenced this pull request Apr 13, 2020
@mark800 @vongosling
[ROCKETMQ-266] Add a specific Exception message for comparing consume…
cea6ef4 
…rThreadMax and consumerThreadMin (apache#147)

* Can’t start consumer with a small consumerThreadMax number

* Can’t start consumer with a small consumerThreadMax number

* test case for ROCKETMQ-266

* Can’t start consumer with a small consumerThreadMax number

* Can’t start consumer with a small consumerThreadMax number

* fix merge conflict

* update test case for ROCKETMQ-266

* for ROCKETMQ-266

* Trigger rebuild

* Trigger rebuild

* Trigger rebuild
JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020
@mark800 @vongosling
[ROCKETMQ-266] Add a specific Exception message for comparing consume…
580049d 
…rThreadMax and consumerThreadMin (apache#147)

* Can’t start consumer with a small consumerThreadMax number

* Can’t start consumer with a small consumerThreadMax number

* test case for ROCKETMQ-266

* Can’t start consumer with a small consumerThreadMax number

* Can’t start consumer with a small consumerThreadMax number

* fix merge conflict

* update test case for ROCKETMQ-266

* for ROCKETMQ-266

* Trigger rebuild

* Trigger rebuild

* Trigger rebuild
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vongosling vongosling vongosling approved these changes

@dongeforever dongeforever dongeforever approved these changes

+1 more reviewer

@Jaskey Jaskey Jaskey left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type/enhancement

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

7 participants

@mark800 @coveralls @vsair @lindzh @dongeforever @vongosling @Jaskey