Skip to content

Conversation

@fuyou001
Copy link
Contributor

@fuyou001 fuyou001 commented May 25, 2018

What is the purpose of the change

thousands of client Sub message, when subscription change, broker notify all client with same cid,
causing related ThreadPoolExecutor pool full, so Heartbeat use independently thread pool

Brief changelog

increase notify consumerIds changed semaphore size

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ROCKETMQ-XXX] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@lizhanhui
Copy link
Contributor

I am wondering why notifying clients of membership change could take up the thread pool? Note notify is performed in an asynchronous non-blocking and one-way manner. Can you share your case details justifying this is indeed the root cause?

@vongosling
Copy link
Member

@fuyou001 thoughts?

@fuyou001
Copy link
Contributor Author

some reasons, I can't say more about it. @lizhanhui @vongosling

@vsair
Copy link
Contributor

vsair commented Jun 28, 2018

@fuyou001 I agree with @lizhanhui , the others command would not take up the thread pool. It's useless to isolate the pool.

@coveralls
Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage increased (+0.003%) to 42.318% when pulling 25aa641 on fuyou001:issues-314 into b054a9d on apache:develop.

@lizhanhui
Copy link
Contributor

lizhanhui commented Jul 13, 2018

Considering heartbeat plays a major role in maintaining cluster members in sync, I'd agree to move it to a dedicated thread-pool so that it won't be delayed too much in case broker is under heavy pressure.

@lizhanhui
Copy link
Contributor

@vsair I talked to @fuyou001 privately via email. The reason he is reluctant to share is that disclosing these technical details may bring about security threats to their commercial service before applying patches to all serving brokers. I'd post updates to share the scenario and explain more at the earliest convenient time.

@lollipopjin
Copy link
Contributor

It seems to be more reasonable to isolate the time-taking notification works with Heartbeat, so +1.

Copy link
Member

@vongosling vongosling left a comment

Choose a reason for hiding this comment

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

LGTM if we have finished the checklist before a merge @fuyou001 Another question, do you consider it is rational if we have created so many threadpool?

@fuyou001
Copy link
Contributor Author

@vongosling

fuyou001 added 2 commits July 16, 2018 13:31
# Conflicts:
#	broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java
@vongosling vongosling merged commit d8c446e into apache:develop Jul 16, 2018
@vongosling vongosling added this to the 4.3.0 milestone Jul 16, 2018
renshuaibing-aaron pushed a commit to renshuaibing-aaron/rocketmq that referenced this pull request Apr 13, 2020
JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants