szetszwo commented on code in PR #1175:
URL: https://github.com/apache/ratis/pull/1175#discussion_r1837078939
##########
ratis-test/src/test/java/org/apache/ratis/server/raftlog/segmented/TestSegmentedRaftLog.java:
##########
@@ -599,18 +600,34 @@ public void testPurgeOnClosedSegmentsWithPurgeGap()
throws Exception {
int segmentSize = 200;
long endIndexOfClosedSegment = segmentSize * (endTerm - startTerm - 1) - 1;
long expectedIndex = RaftLog.LEAST_VALID_LOG_INDEX;
- purgeAndVerify(startTerm, endTerm, segmentSize, 1000,
endIndexOfClosedSegment, expectedIndex);
+ purgeAndVerify(startTerm, endTerm, segmentSize, 1000,
endIndexOfClosedSegment, expectedIndex, 0, 0);
+ }
+
+ @Test
+ public void testPurgeWithLargePurgePreservationAndSmallPurgeGap() throws
Exception {
+ int startTerm = 0;
+ int endTerm = 5;
+ int segmentSize = 200;
+ long endIndex = segmentSize * (endTerm - startTerm) - 1;
+ // start index is set so that the suggested index will not be negative,
which will not trigger any purge
+ long startIndex = 200;
+ // purge preservation is larger than the total size of the log entries
+ // which causes suggested index to be lower than the start index
+ long purgePreservation = (segmentSize * (endTerm - startTerm )) + 100;
+ // if the suggested index is lower than the start index due to the purge
preservation, we should not purge anything
+ purgeAndVerify(startTerm, endTerm, segmentSize, 1, endIndex, startIndex,
startIndex, purgePreservation);
}
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 {
Review Comment:
Let's overload the method. Then, we don't have to change the other calls.
```diff
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);
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -326,6 +326,13 @@ public final CompletableFuture<Long> purge(long
suggestedIndex) {
if (suggestedIndex - lastPurge < purgeGap) {
return CompletableFuture.completedFuture(lastPurge);
}
+ 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);
+ }
Review Comment:
The change looks good. Could you also
- add an `adjustedIndex` (instead of the original code updating the
`suggestedIndex` and having `finalSuggestedIndex`) and
- add more info to the message?
```diff
+++
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);
}
});
}
```
##########
ratis-test/src/test/java/org/apache/ratis/server/raftlog/segmented/TestSegmentedRaftLog.java:
##########
@@ -150,7 +150,7 @@ static SegmentedRaftLog newSegmentedRaftLog(RaftStorage
storage, RaftProperties
.build();
}
- private SegmentedRaftLog newSegmentedRaftLogWithSnapshotIndex(RaftStorage
storage, RaftProperties properties,
+ SegmentedRaftLog newSegmentedRaftLogWithSnapshotIndex(RaftStorage storage,
RaftProperties properties,
Review Comment:
Keep it `private`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
