Add support for time-based snapshot intervals #3918
Conversation
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
Test Results 5 files - 2 5 suites - 2 1m 37s ⏱️ - 1m 3s Results for commit a15f9db. ± Comparison against base commit 1265be4. This pull request removes 47 and adds 34 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
pcholakov
force-pushed
the
pavel/vowpsqrwurmp
branch
from
3492f52 to
ff15f35
Compare
pcholakov
force-pushed
the
pavel/vowpsqrwurmp
branch
from
ff15f35 to
00de3b0
Compare
pcholakov
force-pushed
the
pavel/vowpsqrwurmp
branch
from
00de3b0 to
a15f9db
Compare
pcholakov
requested review from
tillrohrmann
and removed request for
AhmedSoliman
tillrohrmann
approved these changes
Nov 13, 2025
Contributor
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for creating this PR @pcholakov. The changes look good to me. +1 for merging. Do we need to update our docs to also mention the snapshot_interval?
| } | ||
|
|
||
| /// Retrieve the latest known LSN to be archived to the snapshot repository. | ||
| /// Response of `Ok(Lsn::INVALID)` indicates no existing snapshot for the partition. |
Contributor
There was a problem hiding this comment.
Maybe update the comment.
| match self { | ||
| ArchivedLsn::None => Ok(Duration::MAX), | ||
| ArchivedLsn::Snapshot { created_at, .. } => { | ||
| SystemTime::now().duration_since(*created_at) |
Contributor
There was a problem hiding this comment.
Is it important that we distinguish between created_at > now and created_at == now? If not, then we might get rid fo the error case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change adds a basic time interval configuration option for triggering automated snapshots. When both time and record-based intervals are set, the logic for the trigger condition is an "and" between the two, so the record interval becomes a minimum requirement for time-based snapshotting.
Time interval-based snapshotting will get a lot more useful with incremental snapshot support, but this is already a big improvement as it allows operators to configure snapshots in slightly more intuitive terms of elapsed time.
The time interval is measured from the created-at timestamp of the published snapshot, which is based on the wall clock time of the producing node. Clock drift error is assumed to be relatively negligible compared to typical snapshotting intervals. A small amount of jitter is added to each partition's snapshot age check to spread snapshot uploads out over time.
Configuration examples
Periodic + minimum number of records
Translates into: "snapshot every 30 minutes, as long as the applied LSN has advanced by least 10,000 records since the last snapshot".
Periodic (unconditional)
Translates into: "snapshot once every 24 hours even if zero new changes are committed".