Empty store changes - #130
Conversation
| /// | ||
| /// Store-produced roots currently allocate table OIDs sequentially without | ||
| /// deletion, so the next OID can be recovered from the root tables. | ||
| pub fn decode_commit_chunks( |
There was a problem hiding this comment.
So this is actually a wrapper around decode_store_chunks and it prepares the data into Vec<Chunk> which can then be fed into decode_store_chunks? This should probably be placed near store::graph_with_root_commit() method because they are all methods about creating a store, one takes a realm, another takes a bunch of commits.
By the way the table oid allocation logic has changed recently, so you need to rebase.
There was a problem hiding this comment.
Thanks for the heads up about changes on main. I'll rebase my own working tree and see if there is anything worth pushing back here.
There was a problem hiding this comment.
I have renamed the this function to Store::try_from_commit_bytes, should be the same functionality though.
| } | ||
|
|
||
| pub fn abort(self) -> Store { | ||
| self.store |
There was a problem hiding this comment.
It is not sufficient to just return the store as abort. Although the store is not modified until we commit, we need to invalidate the handle when we abort. See txn::inner::TxnInner::invalidate_handles.
I suggest having an abort method in TxnInner, and then have both OwnedTxn and normal Txn's abort method call it.
Also add a test to check that indeed handles are invalidated after calling abort.
There was a problem hiding this comment.
The only thought here is to ensure that following an error within a transaction, the coln store is still available within the automerge-repo handle and another transaction could be attempted.
There was a problem hiding this comment.
Yes, with abort(), you should get back the store unchanged and still be able to use it.
| state: StoreHandleState, | ||
| } | ||
|
|
||
| enum StoreHandleState { |
There was a problem hiding this comment.
Can you document a bit why Option is not sufficient?
There was a problem hiding this comment.
I am afraid I don't consider myself qualified to answer this question.
There was a problem hiding this comment.
If we use an Option then we need to figure out what to do with changes received whilst waiting for the store to become ready. There's an argument to be made that automerge-repo/subduction should handle causal delivery but for the time being they don't. I think the pragmatic thing to do here is to handle the causal delivery within coln-js-runtime, which means we need to queue commits here.
That said, we will also need to queue commits in the event of non-causal delivery even if we're in the Ready state and so maybe the queue should live directly on Store and not in StoreHandleState.
| .ok_or_else(|| js_error("store handle has been moved into a transaction")) | ||
| fn apply_chunks(&mut self, chunk_bytes: Vec<Vec<u8>>) -> Result<(), String> { | ||
| match &mut self.state { | ||
| StoreHandleState::Uninitialized { chunks, has_root } => { |
There was a problem hiding this comment.
Add a bit explanation about the buffering behaviour here. Also add a TODO that this should be moved somewhere else.
There was a problem hiding this comment.
Very happy to add the TODO here. Where do you think the best location for this code would be?
The buffering behaviour shown here is designed to be more of an example and is likely far from optimal. From automerge-repo's perspective, however, the only real requirements are that we can start with an empty initial store and that entries to the hash graph can be applied even should they arrive out of order. From what I understand, this could be at any time, not just on the initial sync. @alexjg may be able to clarify further.
|
@incipit0 Thanks for you review. My apologies, I should have been clearer at the top of the conversation about the intention of this PR. I am by no means a Rust dev and wouldn't claim to have enough understanding over the nuance of the language nor the subtleties of your work on Coln to suggest code which should actually be merged. This code is merely the working model which provides the automerge-repo integration with the required API. I would offer no opinion on the actual design of how this is achieved, but it was felt that showing the mostly LLM produced code might serve as a better method of demonstrating the changes to those who actually know and understand the codebase. I'll add this to the initial comment so that it's clear for anyone else reviewing. On the review points where I feel I may be qualified to comment, I'll reply directly. |
dd86426 to
167296c
Compare
|
See #131 |
Hello @incipit0 @alexjg @mvr @olynch et al
I have been working on getting sync up and running with automerge repo and there are a few changes which I would be most grateful for your input on.
This PR is a largely LLM generated example of what I think is required:
Empty Stores
Firstly, it enables creation of empty stores, which can then be hydrated from the synced commit chunks. This is used to load coln stores of any schema, even if the schema is unknown. It also allows us to buffer out-of-order chunks until the root and dependencies arrive; Automerge Repo and Subduction do not guarantee the arrival order of entries in the hash graph.
Essentially this means that from typescript we can call
StoreHandle.empty()to initialise a store within repo without needing to use the schema.Calling
StoreHandle.heads()in typescript will also return an empty array until the store has loaded, allowing us to determine store readiness.Additional Types
These are some Realm typings which I found in one of the test helpers for
coln-js-runtimeand which I have found useful in the higher level typescript work. Included here for completeness.Transaction recovery
A store should be rolled back if a transaction throws prior to commit. This was an LLM discovery, so I'll let it describe the issue better that I can:
This makes sense to me, but I will leave it to you to determine if this is the correct behaviour.
Please let me know your thoughts and questions when you can.
Many thanks, Alex