libsql: rework sync v2 structure #1820
Conversation
This refactors the new sync code to live in sync.rs. This also includes support for writing metadata to disk to improve syncs on restarts.
LucioFranco
force-pushed
the
lucio/rework-sync-ctx
branch
from
508d725 to
bdb428e
Compare
| // TODO: figure out relation to durable_frame_num | ||
| // let max_frame_no = ctx.max_frame_no(); |
There was a problem hiding this comment.
@penberg before we can merge this would be helpful to clairify the difference between durable_frame_ num and max_frame_no. What I understand is we want to write the durable_frame_num here (which I guess is the max_frame_no from the server) to disk to ensure we start at a more advanced wal frame than 0.
There was a problem hiding this comment.
You always have two different max frame numbers: one on client, one on server. I tend to call the latter durable_frame_num.
| self.set_max_frame_no(max_frame_no).await.unwrap(); | ||
|
|
||
| return Ok(max_frame_no); | ||
| } else if res.status().is_server_error() || attempts < self.max_retries { |
There was a problem hiding this comment.
@penberg small change compared to what you had, I just retry on server failures rather than all failures like auth_token being invalid.
| pub async fn sync_until( | ||
| &self, | ||
| replication_index: FrameNo, | ||
| ) -> Result<crate::database::Replicated> { |
There was a problem hiding this comment.
Please avoid reformattings like that in PRs that are supposed to do something non-trivial.
There was a problem hiding this comment.
Really odd, my editor is auto formatting this but with cargo fmt which we use to check on CI so we avoid these changes its not formatting lines with #[cfg(feature = "")]. Gonna take a look into this, seems really odd that cargo fmt isn't picking this up.
There was a problem hiding this comment.
I wouldn't invest time into that. Just when you see stuff like this put it on a separate PR and even merge it without review.
| let page_size = { | ||
| let rows = conn.query("PRAGMA page_size", crate::params::Params::None)?.unwrap(); | ||
| let rows = conn |
There was a problem hiding this comment.
Again - please avoid noise like that in PRs
| pub fn wal_get_frame(&self, frame_no: u32, page_size: u32) -> Result<BytesMut> { | ||
| let frame_size: usize = 24 + page_size as usize; | ||
|
|
||
| let mut buf = BytesMut::with_capacity(frame_size); |
There was a problem hiding this comment.
Why is this better than just vec![0; frame_size]?
| pub(crate) async fn set_max_frame_no(&mut self, max_frame_no: u32) -> Result<()> { | ||
| // TODO: check if max_frame_no is larger than current known max_frame_no | ||
| self.max_frame_no = max_frame_no; | ||
|
|
||
| self.update_metadata().await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn update_metadata(&mut self) -> Result<()> { | ||
| let path = format!("{}-info", self.db_path); | ||
|
|
||
| let contents = serde_json::to_vec(&MetadataJson { | ||
| max_frame_no: self.max_frame_no, | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| tokio::fs::write(path, contents).await.unwrap(); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn read_and_update_metadata(&mut self) -> Result<()> { | ||
| let path = format!("{}-info", self.db_path); | ||
|
|
||
| let contents = tokio::fs::read(&path).await.unwrap(); | ||
|
|
||
| let metadata = serde_json::from_slice::<MetadataJson>(&contents[..]).unwrap(); | ||
|
|
||
| self.max_frame_no = metadata.max_frame_no; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
What's all of this? Can we have one PR that just switches to hyper and all other changes in another PR please?
|
Yes but since then please post stuff like that in a separate PRs - it costs
little, gets merged sooner and makes the main PR simpler.
…On Mon, 18 Nov 2024 at 16:23, Lucio Franco ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In libsql/src/local/database.rs
<#1820 (comment)>
:
> + pub async fn sync_until(
+ &self,
+ replication_index: FrameNo,
+ ) -> Result<crate::database::Replicated> {
Seems like rustfmt wasn't run on this file (I have it run on save), what
we should have is CI enforcing that we don't merge PRs without it passing
rustfmt checks so I will add that.
—
Reply to this email directly, view it on GitHub
<#1820 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKADLIDVUI7MJGO3VZEVVT2BIBATAVCNFSM6AAAAABRZ2HBP2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBSHE2DAMBYGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
This refactors the new sync code to live in sync.rs. This also includes
support for writing metadata to disk to improve syncs on restarts.