Skip to content

RATIS-2703. Fix GrpcLogAppender SNAPSHOT_EXPIRED pending queue leak - #1597

Open
amaliujia wants to merge 1 commit into
apache:masterfrom
amaliujia:fix_2
Open

amaliujia wants to merge 1 commit into
apache:masterfrom
amaliujia:fix_2

Conversation

@amaliujia

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix GrpcLogAppender.InstallSnapshotResponseHandler to properly handle SNAPSHOT_EXPIRED install-snapshot replies by calling removePending(reply) and setAttemptedToInstallSnapshot(), matching other terminal snapshot results such as SNAPSHOT_UNAVAILABLE and SUCCESS.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2703

How was this patch tested?

Unit test

@amaliujia
amaliujia requested a review from szetszwo September 17, 2026 09:20
@amaliujia

Copy link
Copy Markdown
Contributor Author

R: @szetszwo

@szetszwo szetszwo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amaliujia , thanks for working on this! Please see the comment inlined.

Comment on lines 788 to 795
case SNAPSHOT_EXPIRED:
LOG.warn("{}: Follower failed since the request expired, {}",
name, ServerStringUtils.toInstallSnapshotReplyString(reply));
getFollower().setAttemptedToInstallSnapshot();
removePending(reply);
break;
default:
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's handle SNAPSHOT_EXPIRED the same way as SNAPSHOT_UNAVAILABLE:

diff --git a/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java b/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
index e6870b9b8..60d7ee2b2 100644
--- a/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
+++ b/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
@@ -775,24 +775,26 @@ public class GrpcLogAppender extends LogAppenderBase {
           removePending(reply);
           break;
         case SNAPSHOT_UNAVAILABLE:
-          BatchLogger.print(BatchLogKey.SNAPSHOT_UNAVAILABLE,  name,
-              suffix -> LOG.info("{}: Follower failed since the snapshot is unavailable {}", this, suffix));
-          getFollower().setAttemptedToInstallSnapshot();
-          notifyInstallSnapshotFinished(InstallSnapshotResult.SNAPSHOT_UNAVAILABLE, RaftLog.INVALID_LOG_INDEX);
-          removePending(reply);
-          break;
-        case UNRECOGNIZED:
-          LOG.error("{}: Reply result {}, {}",
-              name, reply.getResult(), ServerStringUtils.toInstallSnapshotReplyString(reply));
+          handleFailureReply(reply, BatchLogKey.SNAPSHOT_UNAVAILABLE);
           break;
         case SNAPSHOT_EXPIRED:
-          LOG.warn("{}: Follower failed since the request expired, {}",
-              name, ServerStringUtils.toInstallSnapshotReplyString(reply));
+          handleFailureReply(reply, BatchLogKey.SNAPSHOT_UNAVAILABLE);
+          break;
         default:
+          LOG.error("{}: Unexpected InstallSnapshotReply result {} in {}",
+              name, reply.getResult(), ServerStringUtils.toInstallSnapshotReplyString(reply));
           break;
       }
     }
 
+    private void handleFailureReply(InstallSnapshotReplyProto reply, BatchLogKey logKey) {
+      BatchLogger.print(logKey,  name,
+          suffix -> LOG.info("{}: Follower failed due to {}: {}{}", this, reply.getResult(), reply, suffix));
+      getFollower().setAttemptedToInstallSnapshot();
+      notifyInstallSnapshotFinished(reply.getResult(), RaftLog.INVALID_LOG_INDEX);
+      removePending(reply);
+    }
+
     @Override
     public void onError(Throwable t) {
       if (!isRunning()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants