-
Notifications
You must be signed in to change notification settings - Fork 158
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
Cancel Snapshot Jobs #596
Comments
👋 Thanks for opening this issue! Get help or engage by:
|
The building snapshot is mainly a tokio task like this: tokio::spawn(
async move {
let f = builder.build_snapshot();
let res = Abortable::new(f, reg).await;
match res {
Ok(res) => match res {
Ok(snapshot) => {...}
Err(err) => {...}
},
Err(_aborted) => {...}
}
}
); It is possible to make it an infinite loop in which it retries calling Another way is to block inside async fn build_snapshot() {
while !is_ready().await {
tokio::time::sleep(Duration::from_millis(10)).await;
}
} Does this approach address your issue? A StorageError is a fatal error. Returning it will just shut RaftCore down at once. Introducing another TryAgainLater error will move this logic out of |
Thanks for the thoughtful explanation and guidance! My initial fear was that such solution would cause tasks to accumulate. With additional clarity from your guidance, it now feels like the obvious solution to me as well. We already wait for state machine backup jobs to complete in the |
I've implemented this approach. Each build snapshot awaits some state machine conditions to be satisfied, which means the I haven't fully investigated the issue, there may be an external cause. I wanted to whether something about the concurrency model changed recently that could explain this behavior. Here's some context,
Then entries stop being applied to the state machine, these lines seem to repeating for a while. This is still a single-node cluster.
A little later in the log, I see indication that the snapshot is in progress.
Just after, I see it trying to apply the next entry to the state machine. My snapshot size is 1000, the request after is 1001. However, the openraft
|
@fredfortier let me see what's going on there 🤔 |
@fredfortier I have been careless in introducing this breaking behavior change :( Let me get this TODO done! |
Before this commit, snapshot is built in the `sm::Worker`, which blocks other state-machine writes, such as applying log entries. This commit parallels applying log entries and building snapshot: A snapshot is built in another `tokio::task`. Because building snapshot is a read operation, it does not have to block the entire state machine. Instead, it only needs a consistent view of the state machine or holding a lock of the state machine. - Fix: databendlabs#596
Before this commit, snapshot is built in the `sm::Worker`, which blocks other state-machine writes, such as applying log entries. This commit parallels applying log entries and building snapshot: A snapshot is built in another `tokio::task`. Because building snapshot is a read operation, it does not have to block the entire state machine. Instead, it only needs a consistent view of the state machine or holding a lock of the state machine. - Fix: databendlabs#596
Thanks for the quick turnaround. I understand the root cause as explained. I'll update and re-validate now. |
If it's helpful, I can contribute some long running snapshots to your test suite if you give me some pointers (where to put it, etc). That's probably the best way to make our usage quirks benefit the protocol. In this case, the state machine conditions that the snapshot is waiting for depends on future normal entries, which is why it was deadlocking. I think your TODO about aborting the snapshot task is important as well. It may be worth considering letting the implementer raise a non-fatal error to either skip the snapshot interval, or retry the snapshot at the last applied log. |
@fredfortier Let's continue further discussing in this issue: |
Regarding this specific issue, your task spawning fixed the issue, but I want to confirm the behavior below is expected:
I assume it's multiple tasks because of the different It does seem to complete properly (not more
|
Hmm. This info level log is inappropriate. It should be debug level. In the function |
Is it possible to cancel a
RaftSnapshotBuilder::build_snapshot
job? In my context, it would simplify things to skip snapshots in particular conditions until the state machine steps into a different state. I simply want the job to try again until a snapshot is successfully returned. I tried returning this error:StorageIOError::new(ErrorSubject::None, ErrorVerb::Write, e.into()).into()
from the leader, but it lead to a reelection instead of retrying. Is there an error kind that would lead to the behavior I'm looking for?The text was updated successfully, but these errors were encountered: