[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #383: RATIS-1273. Fix split brain by leader lease

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

szetszwo commented on a change in pull request #383:
URL: https://github.com/apache/incubator-ratis/pull/383#discussion_r551806316



##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -104,6 +104,20 @@ public RaftPeerRole getCurrentRole() {
       return getRole().getCurrentRole();
     }
 
+    @Override
+    public boolean isLeader() {

Review comment:
       Assume that the leader won't step down if it has lost the leader lease.
   
   If the assumption is correct, then it is better to change isLeaderRead() instead of isLeader().  The client will see LeaderNotReadyException and retry with the same leader.
   
   If we change isLeader(),  the client will see NotLeaderException and retry with a different peer.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -905,6 +906,47 @@ private void yieldLeaderToHigherPriorityPeer() {
     }
   }
 
+  boolean isLeaderLeaseValid() {
+    if (checkLeaderLease()) {
+      return true;
+    }
+
+    updateLeaderLease();
+    return checkLeaderLease();
+  }
+
+  private boolean checkLeaderLease() {
+    return lastLeaderTimestamp.get().elapsedTimeMs() < server.properties().leaderLeaseTimeoutMs();
+  }
+
+  private boolean updateLeaderLease() {
+    Timestamp startLease = Timestamp.currentTime();
+
+    List<RaftPeerId> activePeers = new ArrayList<>();
+    List<FollowerInfo> followerInfos = getFollowerInfos();
+    for (final FollowerInfo info : followerInfos) {
+      if (info.getLastRpcSendTimeWithResponse().elapsedTimeMs() <= server.properties().leaderLeaseTimeoutMs()) {
+        if (startLease.compareTo(info.getLastRpcSendTimeWithResponse()) > 0) {
+          startLease = info.getLastRpcSendTimeWithResponse();
+        }
+        activePeers.add(info.getPeer().getId());
+      }
+    }

Review comment:
       The for-loop will choose all info with condition lastRpcSendTimeWithResponse < leaderLeaseTimeoutMs.  Suppose there are 5 servers (1 leader + 4 followers) and all 4 followers satisfy condition.  The lastRpcSendTimeWithResponse of the followers are 100, 200, 300, 400.  The for-loop will update startLease to the min, i.e. 100.  However, it should update to 300 since 300, 400 and self have the majority.




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