[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2704: HDDS-3227. Ensure eviction of stateMachineData from cache only when both followers catch up

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

lokeshj1703 commented on a change in pull request #2704:
URL: https://github.com/apache/ozone/pull/2704#discussion_r755732321



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -195,6 +200,9 @@ public ContainerStateMachine(RaftGroupId gid, ContainerDispatcher dispatcher,
 
     this.executor = Executors.newFixedThreadPool(numContainerOpExecutors);
     this.containerTaskQueues = new ConcurrentHashMap<>();
+    this.waitOnBothFollowers = conf.getObject(
+        DatanodeConfiguration .class).waitOnAllFollowers();

Review comment:
       NIT: space after DatanodeConfiguration.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -862,6 +892,12 @@ public void notifyTermIndexUpdated(long term, long index) {
     return future;
   }
 
+  @Override
+  public void notifyNotLeader(Collection<TransactionContext> pendingEntries) {
+    // once the leader steps down , clear the cache
+    evictStateMachineCache();

Review comment:
       By default we keep the state machine data in both follower and leader because new leader might need to resend the data.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -736,23 +744,48 @@ public void notifyTermIndexUpdated(long term, long index) {
     return f;
   }
 
+  // Removes the stateMachine data from cache once both followers catch up
+  // to the particular index.
+  private void removeStateMachineDataIfNeeded(long index) {
+    if (waitOnBothFollowers) {

Review comment:
       We will need to handle the case when waitOnBothFollowers is false.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -736,23 +744,48 @@ public void notifyTermIndexUpdated(long term, long index) {
     return f;
   }
 
+  // Removes the stateMachine data from cache once both followers catch up
+  // to the particular index.
+  private void removeStateMachineDataIfNeeded(long index) {
+    if (waitOnBothFollowers) {
+      LOG.info("Removing data corresponding to log index {} from cache",
+          index);
+      try {
+        RaftServer.Division division = ratisServer.getServer().getDivision(gid);
+        if (division.getInfo().isLeader() && Arrays
+            .stream(division.getInfo().getFollowerNextIndices())
+            .allMatch(i -> i >= index)) {
+          LOG.debug("Removing data corresponding to log index {} from cache",
+              index);
+          stateMachineDataCache.remove(index);

Review comment:
       We might not remove some entries in case the condition check returns false.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -736,23 +744,48 @@ public void notifyTermIndexUpdated(long term, long index) {
     return f;
   }
 
+  // Removes the stateMachine data from cache once both followers catch up
+  // to the particular index.
+  private void removeStateMachineDataIfNeeded(long index) {
+    if (waitOnBothFollowers) {
+      LOG.info("Removing data corresponding to log index {} from cache",
+          index);
+      try {
+        RaftServer.Division division = ratisServer.getServer().getDivision(gid);
+        if (division.getInfo().isLeader() && Arrays
+            .stream(division.getInfo().getFollowerNextIndices())
+            .allMatch(i -> i >= index)) {
+          LOG.debug("Removing data corresponding to log index {} from cache",
+              index);
+          stateMachineDataCache.remove(index);
+        }
+      } catch (Exception e) {
+        throw new RuntimeException(e);
+      }
+    }
+  }
+
   /*
    * ApplyTransaction calls in Ratis are sequential.
    */
   @Override
   public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
     long index = trx.getLogEntry().getIndex();
-    // Since leader and one of the followers has written the data, it can
-    // be removed from the stateMachineDataMap.
-    stateMachineDataCache.remove(index);
+   // once both the followers catch up, remove the entry from the cache.

Review comment:
       We can remove this comment.




-- 
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@ozone.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org