[GitHub] [ratis] szetszwo commented on a diff in pull request #702: RATIS-1656. Leftover usage of ForkJoinPool.commonPool() in RaftServerImpl

[GitBox <gi...@apache.org>]

szetszwo commented on code in PR #702:
URL: https://github.com/apache/ratis/pull/702#discussion_r936916794


##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1413,7 +1413,7 @@ leaderId, getMemberId(), currentTerm, followerCommit, state.getNextIndex(), NOT_
             getRaftServer().getPeer());
       }
     }
-    return JavaUtils.allOf(futures).whenCompleteAsync(
+    return JavaUtils.allOf(futures).whenComplete(

Review Comment:
   > Actions supplied for dependent completions of non-async methods may be performed by the thread that completes the current CompletableFuture, or by any other caller of a completion method.
   
   According to the above javadoc https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html , it uses the thread, which is SegmentedRaftLogWorker in this case, completing `futures` passed to `allOf(..)` but not the thread running `appendEntriesAsync`.   Therefore, we should pass serverExecutor as below
   ```
       return JavaUtils.allOf(futures).whenCompleteAsync(
           (r, t) -> followerState.ifPresent(fs -> fs.updateLastRpcTime(FollowerState.UpdateType.APPEND_COMPLETE)),
           serverExecutor
       ).thenApply(v -> {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org