You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/05/16 10:54:49 UTC
[GitHub] [ratis] Xushaohong opened a new pull request, #647: RATIS-1582. Add notify install snapshot finished event to inform the
Xushaohong opened a new pull request, #647:
URL: https://github.com/apache/ratis/pull/647
## What changes were proposed in this pull request?
Currently, the notify install snapshot would not inform when the whole progress is done
From the Ozone side, the statemachine's notifyInstallSnapshotFromLeader is a single request and process. It is fine before we find out that the installation of the snapshot could get stuck due to the whole RocksDB replacement each time (the leader could have purged the raft log during transferring the snapshot and thus triggers another snapshot installation when the previous install request is done). To solve this, we come up with the incremental snapshot idea, which could transfer the incremental part of RocksDB in the next install request, and needs to preserve the checkpoints. The incremental snapshot needs to compare the checkpoints and hence the checkpoints cannot be deleted after the first request to install a snapshot.
The cleanup time of these checkpoints is hard to determine. It is difficult for the follower to tell whether the latest installed snapshot is the last one and apply the logs immediately. The cleanup time depends on the leader's state, and only the leader knows if it is the time to notify the snapshot again or just send append entries. Only when the leader thinks that the follower has already caught up could trigger the cleanup( error case is not included here).
Thus, we shall have an event to help trigger the cleanup of the checkpoints for the Ozone or generally inform the completeness of the install snapshot, which means no more install snapshot requests will be sent and the follower has caught up.
We trigger this event for both the leader and the follower.
As for the leader
1. when the leader receives the snapshot result `SNAPSHOT_INSTALLED`
2. when the leader receives the snapshot result `SNAPSHOT_UNAVAILABLE`
As for the follower
1. when the follower tries appending new entries after successfully installed one snapshot for the first time
2. when the follower knows the statemachine's snapshot is unavailable
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1582
## How was this patch tested?
Manual test for ozone.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893083663
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished(RaftProtos.InstallSnapshotResult result) {}
Review Comment:
> @Xushaohong , Since the new method is for the state machine to clean up, it is useful to pass also the snapshot index; see https://issues.apache.org/jira/secure/attachment/13044817/647_review.patch (apply on top of the change)
Only one place modified, where the initial value should be consistent as -1
`private final AtomicLong installedIndex = new AtomicLong(INVALID_LOG_INDEX);`
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893022183
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished() {}
Review Comment:
> Let's pass the InstallSnapshotResult as a parameter.
Thx for the opinion. Do you mean this?
`default void notifyInstallSnapshotFinished(RaftProtos.InstallSnapshotResult result) {}`
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893024865
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished() {}
Review Comment:
Yes, exactly.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on PR #647:
URL: https://github.com/apache/ratis/pull/647#issuecomment-1127528357
@hanishakoneru @szetszwo PTAL :)
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893061842
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished(RaftProtos.InstallSnapshotResult result) {}
Review Comment:
Since this method is for the state machine to clean up, it is useful to pass also the snapshot index.
BTW, let's rename this to notifySnapshotInstalled as below.
```
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex) {}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/SnapshotInstallationHandler.java:
##########
@@ -314,6 +320,7 @@ private InstallSnapshotReplyProto notifyStateMachineToInstallSnapshot(
LOG.info("{}: InstallSnapshot notification result: {}, at index: {}", getMemberId(),
InstallSnapshotResult.SNAPSHOT_INSTALLED, latestInstalledSnapshotTermIndex);
inProgressInstallSnapshotIndex.set(INVALID_LOG_INDEX);
+ successInstalledSnapshot.set(true);
Review Comment:
Let's also call notifySnapshotInstalled here.
```
server.getStateMachine().event().notifySnapshotInstalled(
InstallSnapshotResult.SNAPSHOT_INSTALLED, installedIndex);
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893083663
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished(RaftProtos.InstallSnapshotResult result) {}
Review Comment:
> @Xushaohong , Since the new method is for the state machine to clean up, it is useful to pass also the snapshot index; see https://issues.apache.org/jira/secure/attachment/13044817/647_review.patch (apply on top of the change)
Updated new commits ~
Only one place modified, where the initial value should be consistent as -1
`private final AtomicLong installedIndex = new AtomicLong(INVALID_LOG_INDEX);`
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on PR #647:
URL: https://github.com/apache/ratis/pull/647#issuecomment-1138117795
> Hi @Xushaohong, thanks for the proposal. I have a question about it.
>
> From ozone's view, if some file in the old checkpoint needs to be purged, doesn't it mean that file is useless?
>
> For example, suppose the old checkpoint is `[1, 2, 3]`, and the new checkpoint is `[1, 3, 4]`. Even if the lagging DN successfully pulls file `2` from master and finished the `NotifyInstallSnapshot`. Next iteration of incremental snapshot will pull `4` and delete `2`. So why do we need to keep `2` on master until `NotifyInstallSnapshot` is done?
>
> Please correct me if I'm wrong.
The Master should keep the checkpoint until the follower truly catches up. Each checkpoint is a complete snapshot of RDB. We don't care about the stale SST files as you mentioned the file ```2``. We keep the checkpoint as a unit to compare, which simplifies the logic.
Master will compare the latest checkpoint and newly-created checkpoint, and send the incremental part. For the stale SSTs, the follower RDB has a mechanism to deal with them, that is using MANIFEST to detect the stale SSTs and GC them.
Basically, the master only needs to have two checkpoints each time for the notify request, we could choose to delete old checkpoints or not, but only deletes all checkpoints after the ratis server tells us the snapshot is finished.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #647:
URL: https://github.com/apache/ratis/pull/647#issuecomment-1137487310
Hi @Xushaohong, thanks for the proposal. I have a question about it.
From ozone's view, if some file in the old checkpoint needs to be purged, doesn't that mean that file is useless?
For example, suppose the old checkpoint is `[1, 2, 3]`, and the new checkpoint is `[1, 3, 4]`.
Even if the lagging DN successfully pulls file `2` from master and finished the `NotifyInstallSnapshot`.
Next iteration of incremental snapshot will pull `4` and delete `2`.
So why do we need to keep `2` on master until `NotifyInstallSnapshot` is done?
Please correct me if I'm wrong.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #647:
URL: https://github.com/apache/ratis/pull/647
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r892504361
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished() {}
Review Comment:
Let's pass the InstallSnapshotResult as a parameter.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] Xushaohong commented on a diff in pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #647:
URL: https://github.com/apache/ratis/pull/647#discussion_r893038258
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -189,6 +189,12 @@ default void notifyGroupRemove() {}
* @param failedEntry The failed log entry, if there is any.
*/
default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
+
+ /**
+ * Notify the {@link StateMachine} that the progress of install snapshot is
+ * completely done. Could trigger the cleanup of snapshots.
+ */
+ default void notifyInstallSnapshotFinished() {}
Review Comment:
I have updated with a new commit. PTAL
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #647: RATIS-1582. Add notify install snapshot finished event to inform the
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #647:
URL: https://github.com/apache/ratis/pull/647#issuecomment-1139340127
Thanks for explaining @Xushaohong. Regardless of how Ozone will use it,
I think it's still useful to inform the leader when `InstallSnapshot` is finished.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org