-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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.
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
[ISSUE #6570] Optimizing the process of putting messages to improve performance #6571
base: develop
Are you sure you want to change the base?
[ISSUE #6570] Optimizing the process of putting messages to improve performance #6571
Conversation
…et in consumeQueue build when the message is illegal
Codecov Report
@@ Coverage Diff @@
## develop #6571 +/- ##
=============================================
+ Coverage 43.10% 43.15% +0.04%
- Complexity 8996 9002 +6
=============================================
Files 1107 1108 +1
Lines 78341 78361 +20
Branches 10216 10225 +9
=============================================
+ Hits 33769 33815 +46
+ Misses 40343 40312 -31
- Partials 4229 4234 +5
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
store/src/main/java/org/apache/rocketmq/store/queue/BatchConsumeQueue.java
Outdated
Show resolved
Hide resolved
This fix is too complex and less performant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimize solution
Hi @lizhanhui, my fix do not change the logic of entering the critical region, but the operation of validate and serialize the messages can indeed be moved outside of topicQueueLock, I will try it. |
Okay. Considering the lock is global, any extra complication should be taken seriously. Looking forward to your optimized solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java
Outdated
Show resolved
Hide resolved
…t_match # Conflicts: # store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java # store/src/main/java/org/apache/rocketmq/store/MessageStore.java # store/src/main/java/org/apache/rocketmq/store/plugin/AbstractPluginMessageStore.java # store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreCleanFilesTest.java # store/src/test/java/org/apache/rocketmq/store/MultiDispatchTest.java
Hi @lizhanhui, I have completed the modification. Moving the serialization operation outside the lock for general messages (messages not related to BCQ and LMQ) is simple, and this is the solution implemented in version 4.x. However, since LMQ and BCQ were introduced after version 5.0, they need to overwrite the queueOffset-related properties during putting message, which caused the current changes. Therefore, for these two types of messages (BCQ and LMQ), when serializing outside the lock, the properties are not written, and properties are serialized and written after the queueOffset is truly determined in lock. This also makes the append process somewhat complicated. I have done some testing and it is currently running normally. Plz feel free to review. |
@lizhanhui Plz help review~ |
@lizhanhui If we don't have enough time to review this PR currently, I suggest submitting another PR that fixes the bug in the original way first. That fix is minimally invasive and follows the current writing process. Then we can further review how to optimize performance in this PR. |
Original way to fix the issue #6641, the main purpose of this PR is to optimize the process of putting messages to improve performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# Conflicts: # store/src/main/java/org/apache/rocketmq/store/CommitLog.java # store/src/main/java/org/apache/rocketmq/store/ConsumeQueue.java # store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java # store/src/main/java/org/apache/rocketmq/store/MessageStore.java # store/src/main/java/org/apache/rocketmq/store/dledger/DLedgerCommitLog.java # store/src/main/java/org/apache/rocketmq/store/plugin/AbstractPluginMessageStore.java # store/src/main/java/org/apache/rocketmq/store/queue/BatchConsumeQueue.java # store/src/main/java/org/apache/rocketmq/store/queue/ConsumeQueueInterface.java # store/src/main/java/org/apache/rocketmq/store/queue/ConsumeQueueStore.java # store/src/test/java/org/apache/rocketmq/store/MultiDispatchTest.java
@@ -815,89 +809,72 @@ public CompletableFuture<PutMessageResult> asyncPutMessage(final MessageExtBroke | |||
} | |||
} | |||
|
|||
topicQueueLock.lock(topicQueueKey); | |||
try { | |||
PutMessageThreadLocal putMessageThreadLocal = this.putMessageThreadLocal.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a race issue? I notice you use thread local variable out of the critical region.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we serialize outside of the lock, so each thread will have a ByteBuf for storing serialized data.
I am kind of busy recently and it is OK to create a new pull request for the optimization and dedicate this one to bugfix. |
TGTM~ |
common/src/main/java/org/apache/rocketmq/common/message/MessageExtBrokerInner.java
Outdated
Show resolved
Hide resolved
store/src/main/java/org/apache/rocketmq/store/MultiDispatch.java
Outdated
Show resolved
Hide resolved
store/src/main/java/org/apache/rocketmq/store/MultiDispatch.java
Outdated
Show resolved
Hide resolved
store/src/main/java/org/apache/rocketmq/store/MultiDispatch.java
Outdated
Show resolved
Hide resolved
store/src/main/java/org/apache/rocketmq/store/ConsumeQueue.java
Outdated
Show resolved
Hide resolved
@caigy Completed the modification of all review comments. |
大概什么时候能合并并发布新版本,谢谢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Make sure set the target branch to
develop
What is the purpose of the change
fix #6570
Brief changelog
We will only increase the queueOffset if the append message is successful.
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR
.[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.