Skip to content

RATIS-1369. When there is no snapshot, the return snapshotIndex should be -1, not 0.#473

Merged
szetszwo merged 3 commits intoapache:masterfrom
GlenGeng-awx:RATIS-1369
Apr 26, 2021
Merged

RATIS-1369. When there is no snapshot, the return snapshotIndex should be -1, not 0.#473
szetszwo merged 3 commits intoapache:masterfrom
GlenGeng-awx:RATIS-1369

Conversation

@GlenGeng-awx
Copy link
Copy Markdown
Contributor

@GlenGeng-awx GlenGeng-awx commented Apr 26, 2021

What changes were proposed in this pull request?

For now, if there is no snapshot existed, the returned snapshotIndex is 0.
It is not correctly, since the raft log starts from index 0 (BTW, in raft paper, the first valid log index is 1, but in ratis, it is 0), a snapshot taken at snapshotIndex 0 should contain the log entry 0. The snapshotIndex indicating an empty/fake snapshot should be -1.

diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
index 728f7e9c..6307ca79 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
@@ -431,7 +431,7 @@ class ServerState implements Closeable {
 
   long getLatestInstalledSnapshotIndex() {
     final TermIndex ti = latestInstalledSnapshot.get();
-    return ti != null? ti.getIndex(): 0L;
+    return ti != null? ti.getIndex(): -1L;
   }
 
   /**
@@ -440,7 +440,7 @@ class ServerState implements Closeable {
    */
   long getSnapshotIndex() {
     final SnapshotInfo s = getLatestSnapshot();
-    final long latestSnapshotIndex = s != null ? s.getIndex() : 0;
+    final long latestSnapshotIndex = s != null ? s.getIndex() : -1;
     return Math.max(latestSnapshotIndex, getLatestInstalledSnapshotIndex());
   }

This issue is found in the test of bootstrap SCM in SCM HA. For details, please check the jira.

What is the link to the Apache JIRA

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

How was this patch tested?

CI

@GlenGeng-awx
Copy link
Copy Markdown
Contributor Author

@bshashikant @bharatviswa504 @szetszwo Can you please take a look ?

@bshashikant
Copy link
Copy Markdown
Contributor

@szetszwo , can you please check this one?

Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a 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. Good catch on the bug.

Let's use RaftLog.INVALID_LOG_INDEX instead of hard coding it. Since it is very minor, I will change it before merging.

@szetszwo szetszwo merged commit 00f0c85 into apache:master Apr 26, 2021
symious pushed a commit to symious/ratis that referenced this pull request Feb 20, 2024
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.

3 participants