-
Notifications
You must be signed in to change notification settings - Fork 407
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-1626. Preserve recent logs when purge logs #688
Conversation
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.
@SzyWilliam Thanks for working on this, left a comment for reference.
return getLong(properties::getLong, PURGE_PRESERVE_LOG_NUM_KEY, PURGE_PRESERVE_LOG_NUM_DEFAULT, getDefaultLog()); | ||
} | ||
static void setPurgePreserveLogNum(RaftProperties properties, int purgePreserveLogNum) { | ||
setInt(properties::setInt, PURGE_PRESERVE_LOG_NUM_KEY, purgePreserveLogNum); |
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.
We keep set and get unified, setInt -> setLong
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.
Done
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.
@SzyWilliam , thanks for working on this! Please see the comments inlined.
final long purgeIndexWithPreserve = Math.max(0L, purgeIndex - purgePreserveLogNum); | ||
raftLog.purge(purgeIndexWithPreserve); |
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.
Let's change it in RaftLogBase.purge(..) so that the new code will be next to the purgeGap code. We may add a purgePreservation
field and use it as below
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java
@@ -301,6 +301,10 @@ public abstract class RaftLogBase implements RaftLog {
@Override
public final CompletableFuture<Long> purge(long suggestedIndex) {
+ if (purgePreservation > 0) {
+ final long currentIndex = getNextIndex() - 1;
+ suggestedIndex = Math.min(suggestedIndex, currentIndex - purgePreservation);
+ }
final long lastPurge = purgeIndex.get();
if (suggestedIndex - lastPurge < purgeGap) {
return CompletableFuture.completedFuture(lastPurge);
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.
Done
@@ -285,6 +285,15 @@ static void setPurgeUptoSnapshotIndex(RaftProperties properties, boolean shouldP | |||
setBoolean(properties::setBoolean, PURGE_UPTO_SNAPSHOT_INDEX_KEY, shouldPurgeUptoSnapshotIndex); | |||
} | |||
|
|||
String PURGE_PRESERVE_LOG_NUM_KEY = PREFIX + "purge.preserve.log.num"; |
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.
"preserve" is a good word choice. Let's call it ".purge. preservation" so that it is similar to ".purge.gap".
BTW, it needs a "." in the beginning.
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.
Done
@codings-dan @szetszwo Thanks for your reviews. I've made corresponding changes. |
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.
(cherry picked from commit 4157bba)
What changes were proposed in this pull request?
Add a conf param to control how many recent logs to preserve when purging log. This param can help application to balance the cost of transferring lagging logs and the cost of transferring a snapshot. By default it is set to 0, so it won't affect current running applications.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1626
How was this patch tested?
It passes all existing unit tests.