Skip to content
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

Initial offline write support #1801

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Initial offline write support #1801

merged 1 commit into from
Nov 14, 2024

Conversation

penberg
Copy link
Collaborator

@penberg penberg commented Oct 30, 2024

This pull request adds a new type of database, synced database, which
allows local writes that can sync to a remote server.

The protocol on server side is simple:

POST /sync/<generation>/<start_frame>/<end_frame>

where request body is the WAL frames.

@penberg penberg force-pushed the push-sync branch 5 times, most recently from 2a1fa88 to 18f4442 Compare November 4, 2024 16:23
@penberg penberg force-pushed the push-sync branch 3 times, most recently from 436679d to d4f6b22 Compare November 13, 2024 18:20
@penberg penberg marked this pull request as ready for review November 13, 2024 19:31
Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some feedback but overall LGTM

async fn push_with_retry(&self, uri: String, auth_token: &Option<String>, frame: Vec<u8>, max_retries: usize) -> Result<()> {
let mut nr_retries = 0;
loop {
let client = reqwest::Client::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the reason for using reqwest instead of just hyper directly? We also probably want to cache this client so we can re-use its dns resolution/tls handshake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other reason than it was easier to get TLS going with reqwest. I don't mind someone rewriting it to use Hyper after this lands in main.

libsql/src/local/database.rs Outdated Show resolved Hide resolved
libsql/src/replication/mod.rs Outdated Show resolved Hide resolved
Comment on lines +3 to +6
pub sync_url: String,
pub auth_token: Option<String>,
pub max_retries: usize,
pub durable_frame_num: u32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should provide getters for these rather than expose them. I can do this in a follow up.

libsql/src/local/connection.rs Show resolved Hide resolved
This patch add initial support for offline writes. To open a local
database that can sync to remote server, use the
Builder::new_synced_database() API.

The protocol on server side is simple:

  **POST** `/sync/<generation>/<start_frame>/<end_frame>`

where request body is the WAL frames.
@penberg penberg added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit cc1d9c5 Nov 14, 2024
17 of 18 checks passed
@penberg penberg deleted the push-sync branch November 14, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants