Closed metanivek closed 11 months ago
Thanks. Simplifying this API to store less state on the server is a good idea.
There is still something off in that API that we could improve. For instance, we have:
type tree ~= hash | concrete_tree
for client/server type batch ~= (path * tree) list
If you blink hard enough, both types should be Merkle proofs (i.e. a tree with some trees hidden behind a hash). I wonder if we could instead expose a client-side API to manipulate Merkle proofs and base all client/server messages on this instead of custom, non-optimised approximations.
I don't have more concrete suggestions apart that I believe you are going in the right direction :-) Also, another thing to consider for the client-side API is how it will look when using other languages (e.g. Python or C). So, we need to check what bindings would look like.
If you blink hard enough, both types should be Merkle proofs (i.e. a tree with some trees hidden behind a hash). I wonder if we could instead expose a client-side API to manipulate Merkle proofs and base all client/server messages on this instead of custom, non-optimised approximations.
The tree part of the batch api definitely could use more exploration (this PR only removes server-side id trees but keeps the rest mostly untouched). If you are adding a pre-existing tree, I think that the api works decently well (just using the tree's key), but if the tree doesn't have a key it falls back to concrete trees, which doesn't seem that great. I'll think about how to unify this using proofs but am open to more thoughts about how it might work if you have them to share!
I think the contents part of the batch works okay. It is either a new value or a hash for an existing value (although maybe it should be contents_key
based instead).
Also, another thing to consider for the client-side API is how it will look when using other languages (e.g. Python or C). So, we need to check what bindings would look like.
Good point. I've exercised it lightly for OCaml through the example, but I agree looking at foreign bindings is a good exercise. I guess a first step would be adding irmin-client
bindings to libirmin
.
I think the initial goal we had with Zach was:
Irmin.S
implementation, and so could just pass irmin-test
I think 1. and 2. are working for irmin-client
, but using the low-level store API calls (so defeating 3.). The batch API was a middle-ground solution to fix 3. but I am not super happy with it either.
Maybe the right way to address this (not in this PR, which is already a reasonable simplification of the batch API) is to look at all the API calls that involve trees
and see what kind of client/server interactions we want from those. I suspect we could re-implement a different tree.ml
for clients, where reads will be on-demand (like they are today) but where we have a way to batch writes. Not totally sure how this would work without looking more in-depth tbh 😅
@art-w thanks for the review! I've updated both the things you called out.
This is a follow up PR from the initial
irmin-server
integration PR.The big changes:
There are several other cleanups: adopting
irmin
's[%log.* ...]
ppx since this is now an officialirmin
sub-project, adding a full example that demonstrates batching, and adding a mutex for requests so that the server is only processing one client request at a time.It's also worth noting that I discovered a bug in
irmin-git
/ocaml-git
while testing the batchapply
API. I opened PR forocaml-git
that fixes the core issue, but until that is released and thenirmin-git
is patched, we cannot use the batch api'sadd_value
withirmin-git
since it results in a decoding issue on the server.