[GitHub] [incubator-ratis] GlenGeng commented on a change in pull request #289: RATIS-1161. Updating commit index of follower does not require term check

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

GlenGeng commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527520976



##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
    * Update the last committed index.
    * @param majorityIndex the index that has achieved majority.
    * @param currentTerm the current term.
+   * @param isLeader Is this server the leader?
    * @return true if update is applied; otherwise, return false, i.e. no update required.
    */
-  public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+  public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
     try(AutoCloseableLock writeLock = writeLock()) {
       final long oldCommittedIndex = getLastCommittedIndex();
       final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
       if (oldCommittedIndex < newCommitIndex) {
-        // Only update last committed index for current term. See §5.4.2 in
-        // paper for details.
+        if (!isLeader) {

Review comment:
       Thanks for the quick fix !
   
   There is a subtle thing here: it should be min of leaderCommit and index of the last new appended entry from this AppendEntriesRequest.
   
   <img width="389" alt="image" src="https://user-images.githubusercontent.com/8412672/99776540-f5202c00-2b4b-11eb-9bf7-a9469961dbe9.png">
   
   Here is the fault scenario:
   
   Given the format of each log entry is <term, index>, 
   
   follower has raft log as
   ``
   <1,1>, <1,2>, <1,3>, <1,4>, <1,5>, <1,6>, 
   ``
   Its flush index is 6, commit index is 3 (which is get from leader of term 1)
   
   leader has raft log as
   ``
   <1,1>, <1,2>, <1,3>, <2,4>, <2,5>
   ``
   Say, its commit index is 4
   
   leader send an empty heartbeat with <previousTerm, previousIndex> as <1,3>, which can pass consistency check.
   
   In this case, follower will advance its commit index to 4, but actually `<1,4>` is not a new added log entry.




----------------------------------------------------------------
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.

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