[ISSUE #1515] SYNC_MASTER could be change into pipeline manner #1516

shenhui0509 · 2019-10-10T09:40:52Z

this commit resolves #1515 change SYNC_MASTER flushSlave/flushDisk to pipeline manner

What is the purpose of the change

performance improvement for SYNC_MASTER :

reduce produce latency
improve produce throughput

Brief changelog

add an interface asyncProcessRequest to remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRequestProcessor.java allow processors can process request in async manner, and by default wrap the result of ProcessRequest with CompletableFuture.
change remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java, let write response to client happen after the future is filled, rather than a blocking call
add and interface asyncPutMessage to MessageStore
change the store/src/main/java/org/apache/rocketmq/store/CommitLog.java, the asyncPutMessage method returns a CompletableFuture, this future is filled by the flushSlave/flushDisk thread instead of the processor thread, thus the processor can process the next request just after append to local commitlog, let the flushSlave/flushDisk thread respond to client.
some dup code eliminate

Verifying this change

Functional

add unit test in store/src/test/java/org/apache/rocketmq/store/HATest.java
shutdown slave while sending message, the producer received the FLUSH_SLAVE_TIMEOUT response code.

performance

benchmark config:
2 * broker：48C，512G mem，4 * 2T SSD; 1-master-1-slave
namesrv is hybrid deployed with broker
3 * client : 40C, 64G mem
message size : 1024B
write thread : 64

benchmark result
sync_pipeline is the optimized version configured with SYNC_MASTER
wait is the origin version configured with SYNC_MASTER
async is the origin version configured with ASYNC_MASTER

TPS

Latency

The result shows that the optimized version's performance is close to ASYNC_MASTER.

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.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github 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 [ISSUE #123] 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(over 80% coverage) 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.

… reduce produce latency and improve throughput

shenhui0509 · 2019-10-10T10:25:06Z

ping @duhenglucky @vongosling

duhenglucky · 2019-10-10T10:55:13Z

@shenhui0509 good job, thanks for your contribution, we will review this PR ASAP.

duhenglucky · 2019-10-10T10:56:07Z

@shenhui0509 and it would be nice if you can pull this request to develop branch

shenhui0509 · 2019-10-10T11:18:59Z

@shenhui0509 and it would be nice if you can pull this request to develop branch

done

shenhui0509 · 2019-10-11T12:34:26Z

Hi, @duhenglucky, is it necessary to write a RIP? If needed, I'd like to write one.

xujianhai666 · 2019-10-11T12:37:36Z

Hi, @duhenglucky, is it necessary to write a RIP? If needed, I'd like to write one.

@shenhui0509 @duhenglucky I think this pr is so important, A RIP is necessary

duhenglucky · 2019-10-12T02:41:57Z

@xujianhai666 good suggestion, @shenhui0509 This PR is indeed a relatively big change, and on the critical path, it is best to write a RIP to describe the scope of the relevant changes, as well as the key design in this PR, if you have any other question, please feel free ask me directly.

remoting/pom.xml

coveralls · 2019-10-14T18:06:53Z

Coverage increased (+0.05%) to 50.534% when pulling b143ff6 on shenhui0509:sync_pipeline into 44569e4 on apache:develop.

RongtongJin

This PR does improve TPS and reduce latency when synchronized replication (Verified by benchmark). There are some issues：

Transaction message does not seem to work after modification.
[DISCUSS] When slave down, master return FLUSH_SLAVE_TIMEOUT instead of SLAVE_NOT_AVALIABLE.

store/src/main/java/org/apache/rocketmq/store/CommitLog.java

2. fix properties when store message 3. add IT for transaction 4. correct resonse code when slave down

shenhui0509 · 2019-10-18T09:33:07Z

This PR does improve TPS and reduce latency when synchronized replication (Verified by benchmark). There are some issues：

Transaction message does not seem to work after modification.

[DISCUSS] When slave down, master return FLUSH_SLAVE_TIMEOUT instead of SLAVE_NOT_AVALIABLE.

I missed msg properties when storage.
I agree to return SLAVE_NOT_AVAILABLE.
Both 1 and 2 are fixed, and add some UT and IT for transaction message. Plz take a look at the new commit

MountainOne · 2019-11-06T08:19:08Z

remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java

-                        final RemotingCommand response = pair.getObject1().processRequest(ctx, cmd);
-                        doAfterRpcHooks(RemotingHelper.parseChannelRemoteAddr(ctx.channel()), cmd, response);
+                        CompletableFuture<RemotingCommand> responseFuture = pair.getObject1().asyncProcessRequest(ctx, cmd);
+                        responseFuture.thenAccept((r) -> {


I think this anonymous variable r should be changed to response for readability

MountainOne · 2019-11-06T08:25:00Z

broker/src/main/java/org/apache/rocketmq/broker/processor/SendMessageProcessor.java

        String originMsgId = MessageAccessor.getOriginMessageId(msgExt);
        MessageAccessor.setOriginMessageId(msgInner, UtilAll.isBlank(originMsgId) ? msgExt.getMsgId() : originMsgId);
+        CompletableFuture<PutMessageResult> putMessageResult = this.brokerController.getMessageStore().asyncPutMessage(msgInner);
+        return putMessageResult.thenApply((r) -> {


I think this anonymous variable r should be changed to response for readability

MountainOne · 2019-11-06T08:28:52Z

broker/src/main/java/org/apache/rocketmq/broker/processor/SendMessageProcessor.java

+                                                                            SendMessageContext sendMessageContext,
+                                                                            ChannelHandlerContext ctx,
+                                                                            int queueIdInt) {
+        return putMessageResult.thenApply((r) ->


I think this anonymous variable r should be changed to putMessageResult for readability

vongosling · 2019-12-06T02:59:21Z

Very great optimization. I will pick some time to work with this pr in the next week.

[ISSUE apache#1515] SYNC_MASTER could be change into pipeline manner

optimization : change SYNC_MASTER flushSlave/flushDisk to pipeline to…

f28be82

… reduce produce latency and improve throughput

shenhui0509 changed the title ~~[improvement] SYNC_MASTER could be change into pipeline manner #1515~~ [improvement] SYNC_MASTER could be change into pipeline manner Oct 10, 2019

duhenglucky added the type/enhancement label Oct 10, 2019

duhenglucky added this to the 4.7.0 milestone Oct 10, 2019

shenhui0509 changed the base branch from master to develop October 10, 2019 11:16

duhenglucky changed the title ~~[improvement] SYNC_MASTER could be change into pipeline manner~~ [ISSUE #1515] SYNC_MASTER could be change into pipeline manner Oct 12, 2019

vongosling reviewed Oct 12, 2019

View reviewed changes

remoting/pom.xml Outdated Show resolved Hide resolved

shenhui.backend added 5 commits October 14, 2019 22:42

WIP: compatible with JDK1.6

d7cafdf

compatible with JDK1.6 in remoting module

a0955a4

fix checkStyle

a785bcf

compatible with dledger

207d4c0

compatible with dledger

6818568

RongtongJin reviewed Oct 15, 2019

View reviewed changes

store/src/main/java/org/apache/rocketmq/store/CommitLog.java Outdated Show resolved Hide resolved

shenhui.backend added 2 commits October 16, 2019 14:01

resolved PR 1516 merge conflicts

f6c05db

1. eliminate redundant CountDownLaunch in GroupCommitRequest

7a6f0ac

2. fix properties when store message 3. add IT for transaction 4. correct resonse code when slave down

MountainOne reviewed Nov 7, 2019

View reviewed changes

duhenglucky mentioned this pull request Nov 11, 2019

RMQ 4.2.0 - 4.5.2 [TIMEOUT_CLEAN_QUEUE] #1585

Closed

resolve conflict

b143ff6

vongosling added the progress/discuss label Dec 6, 2019

RongtongJin force-pushed the develop branch from ad76fd3 to 881f767 Compare January 20, 2020 13:48

duhenglucky modified the milestones: 4.7.0, 4.6.2 Feb 28, 2020

duhenglucky merged commit 64e4ca7 into apache:develop Feb 28, 2020

JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020

Merge pull request apache#1516 from shenhui0509/sync_pipeline

e6af77b

[ISSUE apache#1515] SYNC_MASTER could be change into pipeline manner

GenerousMan pushed a commit to GenerousMan/rocketmq that referenced this pull request Aug 12, 2022

Merge pull request apache#1516 from shenhui0509/sync_pipeline

d740c08

[ISSUE apache#1515] SYNC_MASTER could be change into pipeline manner

pulllock pushed a commit to pulllock/rocketmq that referenced this pull request Oct 19, 2023

Merge pull request apache#1516 from shenhui0509/sync_pipeline

7670197

[ISSUE apache#1515] SYNC_MASTER could be change into pipeline manner

[ISSUE #1515] SYNC_MASTER could be change into pipeline manner #1516

[ISSUE #1515] SYNC_MASTER could be change into pipeline manner #1516

Uh oh!

Conversation

shenhui0509 commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief changelog

Verifying this change

Functional

performance

Uh oh!

shenhui0509 commented Oct 10, 2019

Uh oh!

duhenglucky commented Oct 10, 2019

Uh oh!

duhenglucky commented Oct 10, 2019

Uh oh!

shenhui0509 commented Oct 10, 2019

Uh oh!

shenhui0509 commented Oct 11, 2019

Uh oh!

xujianhai666 commented Oct 11, 2019

Uh oh!

duhenglucky commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RongtongJin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shenhui0509 commented Oct 18, 2019

Uh oh!

MountainOne Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

MountainOne Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

MountainOne Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

vongosling commented Dec 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shenhui0509 commented Oct 10, 2019 •

edited

Loading

duhenglucky commented Oct 12, 2019 •

edited

Loading

coveralls commented Oct 14, 2019 •

edited

Loading