New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RATIS-1482. fix shouldNotifyToInstallSnapshot to return the first avail term index #574
Conversation
…ilable log's start term index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Xushaohong , thanks fa lot for working on this. We don't need to get the latest snapshot as shown below.
+++ b/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
@@ -590,24 +590,16 @@ public class GrpcLogAppender extends LogAppenderBase {
private TermIndex shouldNotifyToInstallSnapshot() {
final FollowerInfo follower = getFollower();
final long leaderNextIndex = getRaftLog().getNextIndex();
+ final long leaderStartIndex = getRaftLog().getStartIndex();
+ final TermIndex firstAvailable = Optional.ofNullable(getRaftLog().getTermIndex(leaderStartIndex))
+ .orElseGet(() -> TermIndex.valueOf(getServer().getInfo().getCurrentTerm(), leaderNextIndex));
final boolean isFollowerBootstrapping = getLeaderState().isFollowerBootstrapping(follower);
-
if (isFollowerBootstrapping && !follower.hasAttemptedToInstallSnapshot()) {
// If the follower is bootstrapping and has not yet installed any snapshot from leader, then the follower should
// be notified to install a snapshot. Every follower should try to install at least one snapshot during
// bootstrapping, if available.
- TermIndex lastEntry = getRaftLog().getLastEntryTermIndex();
- if (lastEntry == null) {
- // lastEntry may need to be derived from snapshot
- SnapshotInfo snapshot = getServer().getStateMachine().getLatestSnapshot();
- if (snapshot != null) {
- lastEntry = snapshot.getTermIndex();
- }
- }
- if (LOG.isDebugEnabled()) {
- LOG.debug("{}: Notify follower to install snapshot as it is bootstrapping to TermIndex {}.", this, lastEntry);
- }
- return lastEntry;
+ LOG.debug("{}: follower is bootstrapping, notify to install snapshot to {}.", this, firstAvailable);
+ return firstAvailable;
}
final long followerNextIndex = follower.getNextIndex();
@@ -615,18 +607,15 @@ public class GrpcLogAppender extends LogAppenderBase {
return null;
}
- final long leaderStartIndex = getRaftLog().getStartIndex();
-
if (followerNextIndex < leaderStartIndex) {
// The Leader does not have the logs from the Follower's last log
// index onwards. And install snapshot is disabled. So the Follower
// should be notified to install the latest snapshot through its
// State Machine.
- return getRaftLog().getTermIndex(leaderStartIndex);
+ return firstAvailable;
} else if (leaderStartIndex == RaftLog.INVALID_LOG_INDEX) {
// Leader has no logs to check from, hence return next index.
- return TermIndex.valueOf(getServer().getInfo().getCurrentTerm(),
- leaderNextIndex);
+ return firstAvailable;
}
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
Outdated
Show resolved
Hide resolved
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
Outdated
Show resolved
Hide resolved
Yours is much more concise. I have submitted a new commit. PTAL @szetszwo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the change looks good.
What changes were proposed in this pull request?
shouldNotifyToInstallSnapshot
should returnthe first available log's start term index
, but right now returnsLastEntryTermIndex
. The possible corner case is when the log is purged and no newer log entries are appended.InstallSnapshotNotificationTests
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1482
How was this patch tested?
CI