Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use latest Mirage_kv.RO signature #559

Merged
merged 4 commits into from Mar 6, 2019

Conversation

Projects
None yet
4 participants
@samoht
Copy link
Member

samoht commented Oct 6, 2018

It's only RO for now, but it is already showing a few issues. The last_modified function is very naive and will iterate over the full store (which is not really optimal :p)

I plan to add the new RW functions too and see how it goes :-)

/cc @linse

@samoht samoht force-pushed the samoht:ro-ng branch from d81c953 to f1cec9d Oct 11, 2018

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Jan 7, 2019

this branch does not compile for us, We fixed some issues in hannesm@e71637b - but since we don't have a deep understanding of irmin internals, we don't know how to properly fix them.

The most recent error is:

File "src/irmin/irmin.ml", line 1:
Error: The implementation src/irmin/irmin.ml
       does not match the interface src/irmin/.irmin.objs/irmin.cmi:
       ...
       At position module type S = <here>
       The value `write_error_t' is required but not provided
       File "src/irmin/irmin.mli", line 2933, characters 2-39:
         Expected declaration
       At position module type S = <here>
       The value `with_tree_exn' is required but not provided
       File "src/irmin/irmin.mli", line 2748, characters 2-252:
         Expected declaration

any chance anyone more familiar with irmin could have a look into rebasing + getting this branch to work? thanks!

@zshipko

This comment has been minimized.

Copy link
Collaborator

zshipko commented Jan 9, 2019

I just had a quick look at rebasing this branch but there have been too many conflicting updates for me to handle it since I'm not that familiar with these changes in particular -- it seems like @samoht is the one who will be able to get this working if anyone can.

@samoht

This comment has been minimized.

Copy link
Member Author

samoht commented Jan 20, 2019

This should be easier to rebase once #609 is merged; I'll have a look at this afternoon.

@samoht samoht force-pushed the samoht:ro-ng branch from f1cec9d to 316f1eb Jan 20, 2019

@samoht

This comment has been minimized.

Copy link
Member Author

samoht commented Jan 20, 2019

I've just rebased that branch.

Note that this is largely untested... /cc @hannesm and @linse

samoht and others added some commits Jan 20, 2019

@linse

This comment has been minimized.

Copy link
Contributor

linse commented Feb 13, 2019

Hallo! We added a sync (git push) to happen after the set, remove and batch operation in the KV interface. This is debatable, maybe the sync should only happen on batch and we should remove our changes for set and remove? Let's discuss!

@samoht

This comment has been minimized.

Copy link
Member Author

samoht commented Feb 13, 2019

Sync on batch sounds better than sync on set. An other option is to either add a new function in the KV_RW api for fsync-like operations, or use one special file in the KV store where you could write to to make a sync (a la 9p). I'm not very fond of both options, so probably adding this to batch is ok..

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Feb 28, 2019

now that mirage-kv (and mirage-kv-lwt) 2.0.0 is in opam repository, i think this can be merged. (may need some lower bounds in the opam file).

@zshipko

This comment has been minimized.

Copy link
Collaborator

zshipko commented Mar 6, 2019

I just tested this branch with the new mirage-kv-lwt release and it seems to be working correctly!

@samoht: is there anything further work needed on this branch? If not I can add the lower-bound for mirage-kv-lwt and merge it.

@samoht samoht merged commit a1c5778 into mirage:master Mar 6, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details

@samoht samoht referenced this pull request Mar 6, 2019

Merged

Improve trees #635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.