irmin
irmin copied to clipboard
Draft: Integrate irmin-server
This PR adds irmin-server
and irmin-client
packages in place of of irmin-http
TODO
- [x] Add copyright headers to new files
- [ ] Decide between keeping optional subpackages or adding
irmin-server-unix
,irmin-client-unix
andirmin-client-jsoo
packages
Codecov Report
Merging #2031 (6e70ff8) into main (020b52e) will increase coverage by
0.02%
. The diff coverage is41.86%
.
:exclamation: Current head 6e70ff8 differs from pull request most recent head bd71dae. Consider uploading reports for the commit bd71dae to get more accurate results
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
+ Coverage 68.84% 68.86% +0.02%
==========================================
Files 132 133 +1
Lines 16165 16208 +43
==========================================
+ Hits 11128 11162 +34
- Misses 5037 5046 +9
Files Changed | Coverage Δ | |
---|---|---|
src/irmin-cli/cli.ml | 21.46% <ø> (ø) |
|
src/irmin-cli/server.ml | 41.86% <41.86%> (ø) |
... and 6 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
@zshipko could you remove the Draft status on this PR? I've force-pushed a rebase - I'm happy to shepherd it until completion. It's a really nice, useful work :-)
Edit: I actually found how to remove the Draft status..
Great! I'm happy to help answer any questions
I've added minimal documentation.
@zshipko I am not sure happy with the current ID management - that seems very error prone (if you forget to call cleanup
on every operation you end up with lots of memory leaks). I reckon it's a good enough solution for now, but I was wondering if you had ideas on how to fix this? I have a few ideas but wanted to check if I haven't forgotten anything :-)
- we can add a
close
operation on the client - that'll automatically callcleanup_all
- we can be a bit more clever on the server-side: generate an ID only for new trees that contains in-memory data - otherwise if no change: keep the same ID - if the tree is already fully committed on disk use its' hash
Ideally it would also be nice if Batch
would actually be the same as the Tree
API, so you could decide to swap those out (if think that's actually a good default!).
Modulo my comments above, this is ready to be merged - the current API subsumes the low-level HTTP API (which should be faster and more reliable). So I plan to delete irmin-http
once this is merged.
This PR adds an interesting (but still needs to complete) higher-level API for efficient tree batching. I'm tempted to either remove it and open another PR with just this to iterate or to mark it as highly experimental until we find the right way to manage remote resources.
@samoht I'm all for merging this and then following up with more improvements. Thanks for picking this up!
- https://github.com/mirage/irmin/pull/1902 has some already started work to remove
irmin-http
. I'm guessing this is the next step - Once this is released we should probably archive https://github.com/mirage/irmin-server/ and update its README. We might want to also move over the README.md and PROTOCOL.md into docs here.
I had some issues with getting the Batch
interface to match Tree
- it definitely seems possible though. The Batch
API is kind of a carry-over from before Irmin.Client.S
implemented Irmin.S
and the whole API was full of slightly different signatures than a typical Irmin store. The idea of a separate batch interface could be avoided by adding more control over when updates are sent to the server (not sure what this would look like exactly) or implementing batching behind the scenes in the Tree
API.
It seems like adding a close
call to the client would be a good start - another idea I considered was to make commands "consuming" which would exchange an ID for a tree and cleanup the ID.
That being said, removing it altogether and coming up with something more sound also seems like it could yield something interesting! Also, with OCaml 5 the way the ID tracking is done is not very nice (it uses a global Hashtbl and global ref) and probably needs some additional considerations anyway.