[GitHub] [ratis] hanishakoneru commented on a change in pull request #573: RATIS-1481. make state upgradate in notifyStateMachineToInstallSnapshot synchronized

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

hanishakoneru commented on a change in pull request #573:
URL: https://github.com/apache/ratis/pull/573#discussion_r782589154



##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -1620,44 +1620,34 @@ private InstallSnapshotReplyProto notifyStateMachineToInstallSnapshot(
         LOG.info("{}: notifyInstallSnapshot: nextIndex is {} but the leader's first available index is {}.",
             getMemberId(), state.getLog().getNextIndex(), firstAvailableLogIndex);
         try {
-          stateMachine.followerEvent().notifyInstallSnapshotFromLeader(roleInfoProto, firstAvailableLogTermIndex)
-              .whenComplete((reply, exception) -> {
-                if (exception != null) {
-                  LOG.warn("{}: Failed to notify StateMachine to InstallSnapshot. Exception: {}",
-                      getMemberId(), exception.getMessage());
-                  inProgressInstallSnapshotRequest.compareAndSet(firstAvailableLogIndex, 0);
-                  return;
-                }
-
-                if (reply != null) {
-                  LOG.info("{}: StateMachine successfully installed snapshot index {}. Reloading the StateMachine.",
-                      getMemberId(), reply.getIndex());
-                  stateMachine.pause();
-                  state.updateInstalledSnapshotIndex(reply);
-                  state.reloadStateMachine(reply.getIndex());
-                  installedSnapshotIndex.set(reply.getIndex());
-                } else {
-                  isSnapshotNull.set(true);
-                  if (LOG.isDebugEnabled()) {
-                    LOG.debug("{}: StateMachine could not install snapshot as it is not available", this);
-                  }
-                }
-              // wait for 1 seconds for statemachine to install snapshot
-              }).get(1, TimeUnit.SECONDS);
-        } catch (InterruptedException | TimeoutException t) {
-          //nothing to do
-        } catch (Exception t) {
-          // there are two cases:
-          //1 `get()` may throw ExecutionException if `whenComplete` throw an exception
-          //2 when generating completeFuture, `statemachine#notifyInstallSnapshotFromLeader`
+          CompletableFuture<TermIndex> future = stateMachine.followerEvent()
+              .notifyInstallSnapshotFromLeader(roleInfoProto, firstAvailableLogTermIndex);
+          // wait for at most 5 seconds for statemachine to install snapshot
+          TermIndex reply = future.get(RaftServerConfigKeys.Notification.notifyInstallSnapshotTimeout(
+              getRaftServer().getProperties()).toLong(TimeUnit.MILLISECONDS), TimeUnit.MILLISECONDS);

Review comment:
       We are essentially going to be blocking the main thread for 5 seconds (or more if configured that way). I don't think we should be blocking the main thread.
   Also, inProgressInstallSnapshotRequest is not being set to 0 when the future completes. 




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