Upgrade to automerge 0.5; fix compaction #58

teohhanhui · 2023-11-11T13:07:44Z

Replaces #21 and #50

TODO:

Is ee9504d needed? (/cc @issackelly)

src/repo.rs

src/fs_store.rs

issackelly · 2023-11-12T20:08:26Z

@teohhanhui I think that's the one where I chose Greg's API over Alex's for compaction and nobody has really stepped in to decide which one was more correct but we do need them to be consistent for sure.

#52 (comment) is the context

alexjg

@issackelly apologies for missing the compaction API stuff. I think what you have here makes sense. I'm actually busy working on a bunch of parts of this codebase getting it to interop with the JS implementation and I might tighten up the storage API a bit as part of that.

src/repo.rs

src/fs_store.rs

src/repo.rs

alexjg · 2023-11-24T11:54:30Z

src/repo.rs

@@ -732,7 +766,7 @@ impl DocumentInfo {
        }
        let waker = Arc::new(RepoWaker::Storage(wake_sender.clone(), document_id));
        self.state.poll_pending_save(waker);
-        self.patches_since_last_save = 0;
+        self.last_heads = new_heads;


Can this lead to data loss? We update the heads of the last time we saved before the storage future has resolved, which means the storage future can fail for some reason (or more likely, be stuck in some kind of deadlock) and then then next time we save we only save since the heads which havent made it to disk.

I'm not sure about whether it can lead to data loss, but in any case it does make sense to store the heads together with the future onto the DocState, and then note_changes could use the heads of the last (fut, heads) pair? I think it's a good follow-up, because it's a complicated change to make.

src/repo.rs

gterzian · 2023-11-27T07:12:17Z

src/repo.rs

@@ -732,7 +766,7 @@ impl DocumentInfo {
        }
        let waker = Arc::new(RepoWaker::Storage(wake_sender.clone(), document_id));
        self.state.poll_pending_save(waker);
-        self.patches_since_last_save = 0;
+        self.last_heads = new_heads;


I'm not sure about whether it can lead to data loss, but in any case it does make sense to store the heads together with the future onto the DocState, and then note_changes could use the heads of the last (fut, heads) pair? I think it's a good follow-up, because it's a complicated change to make.

src/repo.rs

teohhanhui · 2023-11-28T15:48:22Z

@gterzian I hope I didn't mess up the rebase for the test_document_changed_over_sync 😅

b55cf0f seems no longer necessary? So I have squashed it.

gterzian · 2023-11-29T07:24:30Z

@teohhanhui Thanks! Will take another look...

gterzian · 2023-12-01T07:24:36Z

Ok if we remove the line at #58 (comment) and fix the conflict it looks good to go. Haven't looked at the fs store changes yet. cc @alexjg

Keep a thread local counter for number of changes before compaction.

…c didn't change since last save

teohhanhui · 2023-12-05T15:30:37Z

Punting the question of ee9504d, but otherwise this should be ready to merge if everything is okay.

teohhanhui commented Nov 11, 2023

View reviewed changes