You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "ivandika3 (via GitHub)" <gi...@apache.org> on 2026/05/02 14:08:48 UTC
[PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
ivandika3 opened a new pull request, #1444:
URL: https://github.com/apache/ratis/pull/1444
## What changes were proposed in this pull request?
When Ratis follower is installing snapshot, its appliedIndex will not advance until the snapshot is installed. This can cause all linearizable follower read on this node to timeout due to the client timeout (default is 3s).
Instead of waiting until the client timeout, Ratis follower can check whether it is currently installing snapshot and trigger failover immediately if it does.
Ideally, we might also need to fail any pending read requests.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2511
## How was this patch tested?
Integration test.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1444:
URL: https://github.com/apache/ratis/pull/1444#discussion_r3237626570
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1092,12 +1092,11 @@ private CompletableFuture<RaftClientReply> staleReadAsync(RaftClientRequest requ
}
return processQueryFuture(stateMachine.queryStale(request.getMessage(), minIndex), request);
}
-
- ReadRequests getReadRequests() {
- return getState().getReadRequests();
- }
-
private CompletableFuture<ReadIndexReplyProto> sendReadIndexAsync(RaftClientRequest clientRequest) {
+ final Throwable snapshotInstallation = snapshotInstallationHandler.getInProgressInstallSnapshotReadException();
Review Comment:
Let's create the ReadException with a different message. It will be easier to debug later.
```java
ReadException newReadException(String op, long installSnapshot, boolean started) {
return new ReadException(getMemberId() + ": Failed to " + op + " readIndex as snapshot (" + installSnapshot
+ ") installation is " + (started ? "started" : "in progress"));
}
private CompletableFuture<ReadIndexReplyProto> sendReadIndexAsync(RaftClientRequest clientRequest) {
final long installSnapshot = snapshotInstallationHandler.getInProgressInstallSnapshotIndex();
if (installSnapshot != RaftLog.INVALID_LOG_INDEX) {
return JavaUtils.completeExceptionally(newReadException("get", installSnapshot, false));
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1146,7 +1145,8 @@ private CompletableFuture<RaftClientReply> readAsync(RaftClientRequest request)
}
return replyFuture
- .thenCompose(readIndex -> getReadRequests().waitToAdvance(readIndex))
+ .thenCompose(readIndex -> getState().getReadRequests().waitToAdvance(readIndex,
+ snapshotInstallationHandler::getInProgressInstallSnapshotReadException))
Review Comment:
Let's add a waitReadIndex method and check there.
```java
.thenCompose(this::waitReadIndex)
```
```java
private CompletableFuture<Long> waitReadIndex(long readIndex) {
final long installSnapshot = snapshotInstallationHandler.getInProgressInstallSnapshotIndex();
if (installSnapshot != RaftLog.INVALID_LOG_INDEX) {
return JavaUtils.completeExceptionally(newReadException("start waiting for", installSnapshot, false));
}
return getState().getReadRequests().waitToAdvance(readIndex);
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ReadRequests.java:
##########
@@ -88,6 +101,14 @@ private void handleTimeout(long readIndex) {
removed.completeExceptionally(new ReadException("Read timeout " + readTimeout + " for index " + readIndex));
}
+ void fail(Throwable cause) {
+ final Collection<CompletableFuture<Long>> futures;
+ synchronized (this) {
+ futures = new ArrayList<>(sorted.values());
+ sorted.clear();
Review Comment:
- Let's make sorted non-final in order to avoid the copying.
- We may return the futures here and fail them in ReadRequests.fail(..)..
```java
//ReadIndexQueue
synchronized Collection<CompletableFuture<Long>> clear(Throwable cause) {
final Collection<CompletableFuture<Long>> futures = sorted.values();
sorted = new TreeMap<>();
return futures;
}
```
```java
//ReadRequests
void fail(Throwable cause) {
for(CompletableFuture<Long> f : readIndexQueue.clear(cause)) {
f.completeExceptionally(cause);
}
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ReadRequests.java:
##########
@@ -20,21 +20,30 @@
import org.apache.ratis.conf.RaftProperties;
import org.apache.ratis.protocol.exceptions.ReadException;
import org.apache.ratis.server.RaftServerConfigKeys;
+import org.apache.ratis.util.JavaUtils;
import org.apache.ratis.util.Preconditions;
import org.apache.ratis.util.TimeDuration;
import org.apache.ratis.util.TimeoutExecutor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.util.ArrayList;
+import java.util.Collection;
import java.util.NavigableMap;
import java.util.TreeMap;
import java.util.concurrent.CompletableFuture;
import java.util.function.LongConsumer;
+import java.util.function.Supplier;
/** For supporting linearizable read. */
class ReadRequests {
private static final Logger LOG = LoggerFactory.getLogger(ReadRequests.class);
+ static ReadException newException(Object server, long installSnapshot) {
+ return new ReadException(server + ": Failed read as snapshot (" + installSnapshot
+ + ") installation is in progress");
+ }
Review Comment:
Let's move this method to `RaftServerImpl.newReadException` as mentioned earlier.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ReadRequests.java:
##########
@@ -52,10 +61,14 @@ static class ReadIndexQueue {
this.readTimeout = readTimeout;
}
- CompletableFuture<Long> add(long readIndex) {
+ CompletableFuture<Long> add(long readIndex, Supplier<Throwable> failureSupplier) {
final CompletableFuture<Long> returned;
final boolean create;
synchronized (this) {
+ final Throwable failure = failureSupplier.get();
+ if (failure != null) {
+ return JavaUtils.completeExceptionally(failure);
+ }
Review Comment:
This can be checked in RaftServerImpl.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1444:
URL: https://github.com/apache/ratis/pull/1444#discussion_r3238969148
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1146,7 +1145,8 @@ private CompletableFuture<RaftClientReply> readAsync(RaftClientRequest request)
}
return replyFuture
- .thenCompose(readIndex -> getReadRequests().waitToAdvance(readIndex))
+ .thenCompose(readIndex -> getState().getReadRequests().waitToAdvance(readIndex,
+ snapshotInstallationHandler::getInProgressInstallSnapshotReadException))
Review Comment:
Thanks for the review. Updated, but there might be a small chance of interleaving here since the snapshot checking is not protected by ReadRequests object lock.
1. waitReadIndex() checks snapshot state and sees no snapshot.
1. Snapshot notification sets inProgressInstallSnapshotIndex.
1. Snapshot notification calls ReadRequests.fail() and clears the current queue.
1. Read then enqueues itself in ReadRequests.waitToAdvance() after the failure sweep.
We can probably synchronized on RaftServerImpl lock, but I'm afraid this might increase follower read contention.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1444:
URL: https://github.com/apache/ratis/pull/1444
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1444:
URL: https://github.com/apache/ratis/pull/1444#issuecomment-4485170481
> This branch has not been deployed
> No deployments
Not sure why it did not run the checks and the tests?
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1444:
URL: https://github.com/apache/ratis/pull/1444#discussion_r3244182726
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -1146,7 +1145,8 @@ private CompletableFuture<RaftClientReply> readAsync(RaftClientRequest request)
}
return replyFuture
- .thenCompose(readIndex -> getReadRequests().waitToAdvance(readIndex))
+ .thenCompose(readIndex -> getState().getReadRequests().waitToAdvance(readIndex,
+ snapshotInstallationHandler::getInProgressInstallSnapshotReadException))
Review Comment:
@ivandika3 , That is a good point! Then, let's use your previous approach then; see https://issues.apache.org/jira/secure/attachment/13082234/1444_review2.patch
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #1444:
URL: https://github.com/apache/ratis/pull/1444#issuecomment-4468921737
@szetszwo Thanks for checking, forgot to push it. I have addressed the comment in the latest commit.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #1444:
URL: https://github.com/apache/ratis/pull/1444#issuecomment-4493680967
Thanks @szetszwo for the review and triggering the CI.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #1444:
URL: https://github.com/apache/ratis/pull/1444#issuecomment-4485183710
> Not sure why it did not run the checks and the tests?
Thanks @szetszwo for the review. You can refer to https://github.com/apache/ratis/pull/1448#issuecomment-4393394029 , my GH action is still blocked so any commit written by me won't trigger Github actions.
One way is you can help to trigger an empty commit to trigger CI.
--
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
Re: [PR] RATIS-2511. Follower should throw ReadException if it is installing snapshot [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1444:
URL: https://github.com/apache/ratis/pull/1444#issuecomment-4462224428
@ivandika3 , not sure if you have seen [this comment](https://github.com/apache/ratis/pull/1444#discussion_r3244182726)?
--
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