feat: s3 replica and pitr #17
Conversation
timemachine/src/time_travel.rs
Outdated
| ) without rowid; | ||
| create view kv as | ||
| select k, v, v_encoding, version, seq, expiration_ms from kv_snapshot; | ||
| insert into migration_state (k, version) values (0, 3); |
There was a problem hiding this comment.
Why is this 3? To satisfy sqlite.rs?
There was a problem hiding this comment.
yes, although currenty sqlite.rs does not check migration state for read-only databases
There was a problem hiding this comment.
If it doesn't, I'd remove this.
There was a problem hiding this comment.
I have changed migration_state to a view to prevent accidental migration. We may still want to check the migration state in the future (for example if we have a breaking change to kv table), so I'd keep this...
| #[derive(Parser)] | ||
| pub enum SubCmd { | ||
| /// Starts the Deno KV HTTP server. | ||
| Serve(ServeOptions), |
There was a problem hiding this comment.
It'd be nice to make this the default if no subcommand is specified.
There was a problem hiding this comment.
then there would be two ways to serve and this is confusing...
| // If we haven't yet fetched the snapshot range list, fetch it now. | ||
| // False-positive is possible if the remote database is empty, but it's fine. | ||
| let maybe_ranges = list_initial_snapshot_ranges(&tx)?; | ||
| ranges = if maybe_ranges.is_empty() { | ||
| let ranges = source.list_snapshot_ranges().await?; | ||
| insert_initial_snapshot_ranges(&tx, &ranges)?; | ||
| list_initial_snapshot_ranges(&tx)? | ||
| } else { | ||
| maybe_ranges | ||
| }; |
There was a problem hiding this comment.
This means that we'll only sync the initial snapshot list once. Running this code again does nothing. Log->snapshot compaction is not supported in this format version, so this is fine (no possibility of age related de-syncs).
denokv/main.rs
Outdated
| match &config.subcommand { | ||
| SubCmd::Serve(options) => { | ||
| if options.read_only && options.sync_from_s3 { | ||
| anyhow::bail!("Cannot sync from S3 in read-only mode."); |
There was a problem hiding this comment.
Clap can already enforce this, see above.
Can you add a check that none of the options.replica are specified when sync_from_s3 is unset?
There was a problem hiding this comment.
Can you add a check that none of the options.replica are specified when sync_from_s3 is unset?
The S3 options are also taken from env vars, so checking this would break if the user has dirty environment variables, for example when running in docker exec inside Docker.
This check is already in |
No description provided.