RATIS-2186. Raft log should not purge index lower than the log start index#1175
Merged
szetszwo merged 4 commits intoapache:masterfrom Nov 12, 2024
Merged
RATIS-2186. Raft log should not purge index lower than the log start index#1175szetszwo merged 4 commits intoapache:masterfrom
szetszwo merged 4 commits intoapache:masterfrom
Conversation
Contributor
Author
|
cc: @szetszwo @SzyWilliam |
szetszwo
reviewed
Nov 11, 2024
Contributor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@ivandika3 , thanks a lot for working on this!
- Let's also change
SegmentedRaftLogCacheto handle the -1 case, which is the same as thetoDeletelist being empty.
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java
@@ -364,6 +364,9 @@ public class SegmentedRaftLogCache {
list.addAll(segments);
segments.clear();
sizeInBytes = 0;
+ } else if (segmentIndex == -1) {
+ // nothing to purge
+ return null;
} else if (segmentIndex >= 0) {
// we start to purge the closedSegments which do not overlap with index.
LogSegment overlappedSegment = segments.get(segmentIndex);- See also the comments inlined and https://issues.apache.org/jira/secure/attachment/13072740/1175_review.patch
Comment on lines
+329
to
+335
| long startIndex = getStartIndex(); | ||
| if (suggestedIndex < startIndex) { | ||
| LOG.info("{}: purge is skipped since the suggested index {} is lower than " + | ||
| "log start index {}", | ||
| getName(), suggestedIndex, startIndex); | ||
| return CompletableFuture.completedFuture(lastPurge); | ||
| } |
Contributor
There was a problem hiding this comment.
The change looks good. Could you also
- add an
adjustedIndex(instead of the original code updating thesuggestedIndexand havingfinalSuggestedIndex) and - add more info to the message?
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java
@@ -318,20 +318,28 @@ public abstract class RaftLogBase implements RaftLog {
@Override
public final CompletableFuture<Long> purge(long suggestedIndex) {
+ final long adjustedIndex;
if (purgePreservation > 0) {
final long currentIndex = getNextIndex() - 1;
- suggestedIndex = Math.min(suggestedIndex, currentIndex - purgePreservation);
+ adjustedIndex = Math.min(suggestedIndex, currentIndex - purgePreservation);
+ } else {
+ adjustedIndex = suggestedIndex;
}
final long lastPurge = purgeIndex.get();
- if (suggestedIndex - lastPurge < purgeGap) {
+ if (adjustedIndex - lastPurge < purgeGap) {
+ return CompletableFuture.completedFuture(lastPurge);
+ }
+ final long startIndex = getStartIndex();
+ if (adjustedIndex < startIndex) {
+ LOG.info("{}: purge({}) is skipped: adjustedIndex = {} < startIndex = {}, purgePreservation = {}",
+ getName(), suggestedIndex, adjustedIndex, startIndex, purgePreservation);
return CompletableFuture.completedFuture(lastPurge);
}
- LOG.info("{}: purge {}", getName(), suggestedIndex);
- final long finalSuggestedIndex = suggestedIndex;
- return purgeImpl(suggestedIndex).whenComplete((purged, e) -> {
+ LOG.info("{}: purge {}", getName(), adjustedIndex);
+ return purgeImpl(adjustedIndex).whenComplete((purged, e) -> {
updatePurgeIndex(purged);
if (e != null) {
- LOG.warn(getName() + ": Failed to purge " + finalSuggestedIndex, e);
+ LOG.warn(getName() + ": Failed to purge " + adjustedIndex, e);
}
});
}
Contributor
Author
There was a problem hiding this comment.
Thanks for the review. A lot clearer now. Updated as per the suggestion.
| } | ||
|
|
||
| private SegmentedRaftLog newSegmentedRaftLogWithSnapshotIndex(RaftStorage storage, RaftProperties properties, | ||
| SegmentedRaftLog newSegmentedRaftLogWithSnapshotIndex(RaftStorage storage, RaftProperties properties, |
Comment on lines
621
to
+622
| private void purgeAndVerify(int startTerm, int endTerm, int segmentSize, int purgeGap, long purgeIndex, | ||
| long expectedIndex) throws Exception { | ||
| List<SegmentRange> ranges = prepareRanges(startTerm, endTerm, segmentSize, 0); | ||
| long expectedIndex, long startIndex, long purgePreservation) throws Exception { |
Contributor
There was a problem hiding this comment.
Let's overload the method. Then, we don't have to change the other calls.
private void purgeAndVerify(int startTerm, int endTerm, int segmentSize, int purgeGap, long purgeIndex,
long expectedIndex) throws Exception {
- List<SegmentRange> ranges = prepareRanges(startTerm, endTerm, segmentSize, 0);
+ purgeAndVerify(startTerm, endTerm, segmentSize, purgeGap, purgeIndex, expectedIndex, 0, 0);
+ }
+
+ private void purgeAndVerify(int startTerm, int endTerm, int segmentSize, int purgeGap, long purgeIndex,
+ long expectedIndex, long startIndex, long purgePreservation) throws Exception {
+ List<SegmentRange> ranges = prepareRanges(startTerm, endTerm, segmentSize, startIndex);
List<LogEntryProto> entries = prepareLogEntries(ranges, null);
Contributor
Author
|
@szetszwo Thanks for the review and the suggestions. I have updated them accordingly.
This is a good idea to prevent similar issue when calling |
szetszwo
approved these changes
Nov 12, 2024
Contributor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
Contributor
|
@ivandika3 , thanks a lot for working on this! @OneSizeFitsQuorum , thanks for reviewing this! |
Contributor
Author
|
@szetszwo @OneSizeFitsQuorum Thanks for the reviews and suggestions. |
OneSizeFitsQuorum
pushed a commit
to OneSizeFitsQuorum/ratis
that referenced
this pull request
Nov 18, 2024
SzyWilliam
pushed a commit
that referenced
this pull request
Jan 9, 2025
SzyWilliam
pushed a commit
to SzyWilliam/ratis
that referenced
this pull request
Jan 9, 2025
szetszwo
pushed a commit
that referenced
this pull request
Jan 9, 2025
szetszwo
pushed a commit
to szetszwo/ratis
that referenced
this pull request
May 23, 2025
szetszwo
pushed a commit
to szetszwo/ratis
that referenced
this pull request
May 23, 2025
slfan1989
pushed a commit
to slfan1989/ratis
that referenced
this pull request
May 6, 2026
slfan1989
pushed a commit
to slfan1989/ratis
that referenced
this pull request
May 6, 2026
…index (apache#1175) (cherry picked from commit 13b8cdd)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Please refer to https://issues.apache.org/jira/browse/RATIS-2186
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2186
How was this patch tested?
Unit test.