IGNITE-24484 RAFT heartbeat coalescing #5221
Conversation
| @@ -88,19 +93,78 @@ public Future<Message> requestVote(final PeerId peerId, final RequestVoteRequest | |||
|
|
|||
| @Override | |||
| public Future<Message> appendEntries(final PeerId peerId, final AppendEntriesRequest request, | |||
| final int timeoutMs, final RpcResponseClosure<AppendEntriesResponse> done) { | |||
| final int timeoutMs, final RpcResponseClosure<AppendEntriesResponse> done) { | |||
There was a problem hiding this comment.
Formatting issues here and below.
There was a problem hiding this comment.
It was formatted by our rules.
There was a problem hiding this comment.
Our rules forbid reformatting of jraft package completely
| @@ -198,7 +198,7 @@ void executesWrongJobClassOnRemoteNodesAsync(String jobClassName, int errorCode, | |||
| JobTarget.anyNode(clusterNode(node(1)), clusterNode(node(2))), | |||
| JobDescriptor.builder(jobClassName).units(units()).build(), | |||
| null | |||
| ).get(1, TimeUnit.SECONDS)); | |||
| ).get(10, TimeUnit.SECONDS)); | |||
There was a problem hiding this comment.
Why is this required ?
There was a problem hiding this comment.
Computer tasks become slower. I do not think exactly why, but running any task takes more time than before the patch. And also compute that sute on TC becomes longer.
There was a problem hiding this comment.
This is definintely a blocker for PR merging.
|
|
||
| if (connect(peerId)) { // Replicator should be started asynchronously by node joined event. | ||
| if (isHeartbeatRequest(request)) { |
There was a problem hiding this comment.
This check is already done in the upper level in
org.apache.ignite.raft.jraft.core.Replicator#sendEmptyEntries(boolean, org.apache.ignite.raft.jraft.rpc.RpcResponseClosure<org.apache.ignite.raft.jraft.rpc.RpcRequests.AppendEntriesResponse>, boolean)
Just call sendHeartbeat from this method.
if (isHeartbeat) {
....
this.heartbeatInFly = this.rpcService.sendHeartbeat(this.options.getPeerId().getEndpoint(), request, coalesce,
this.options.getElectionTimeoutMs() / 2, heartbeatDone);
}
There was a problem hiding this comment.
I know, but it would be more complicated due to component dependencies. We already use this manner of
checking in AppendEntriesRequestProcessor.
There was a problem hiding this comment.
It was implemented this way in the PoC without any dependency issues.
Ok. let's keep it as is, but at least remove copypaste by moving isHearbeat(..) to utitlities class.
| * @param request Append entries request. | ||
| * @return True if that request is heartbeat or false otherwise. | ||
| */ | ||
| private static boolean isHeartbeatRequest(final AppendEntriesRequest request) { |
There was a problem hiding this comment.
This is not needed. See the comment above.
There was a problem hiding this comment.
The same as AppendEntriesRequestProcessor#isHeartbeatRequest
| final Executor rpcExecutor) { | ||
| final RpcClient rc = this.rpcClient; | ||
| public <T extends Message> CompletableFuture<Message> invokeWithDone( | ||
| PeerId peerId, |
There was a problem hiding this comment.
Avoid reformatting raft code.
There was a problem hiding this comment.
I applied formatting that is used in our entire project.
There was a problem hiding this comment.
Formatting of jraft code is forbidden by style rules.
| * | ||
| * @return The future. | ||
| */ | ||
| CompletableFuture<Message> invokeAsync( |
There was a problem hiding this comment.
I was not able to understand why this interface is introduced.
There was a problem hiding this comment.
The interface is requested to use it for coalessing requests. This interface allows us to not immediately send messages to the network. Instead, you can handle the message manually.
org.apache.ignite.raft.jraft.rpc.impl.core.DefaultRaftClientService#sendHeartbeat
| scheduler = opts.getScheduler(); | ||
| messagesFactory = opts.getRaftMessagesFactory(); | ||
|
|
||
| scheduler.schedule(this::onSentHeartbeat , opts.getElectionTimeoutMs(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
This scheme is not efficient.
Currently each raft group generate it's own timer event in r.startHeartbeatTimer(Utils.nowMs());
Which means events multiplication linear to groups number and excessive CPU usage.
Instead, a single event per all groups can be triggered.
There was a problem hiding this comment.
Probably we can create another ticket for this? Or you insist to do this improvement in this one.
There was a problem hiding this comment.
I'm ok with another ticket.
vldpyatkov
force-pushed
the
ignite-24484
branch
from
7e8bf95 to
788341c
Compare
https://issues.apache.org/jira/browse/IGNITE-24484