-
Notifications
You must be signed in to change notification settings - Fork 14
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
Usefulness of batch
#36
Comments
hannesm
added a commit
to hannesm/mirage-kv
that referenced
this issue
Dec 1, 2022
Thanks for raising your concerns. in the previous mirage meeting we settled to remove |
hannesm
added a commit
to hannesm/opam-repository
that referenced
this issue
Dec 12, 2022
CHANGES: * Use ptime directly for RO.last_modified, instead of the int * int64 pair (mirage/mirage-kv#34) * Add RW.allocate to allocate a key and fill it with zero bytes (mirage/mirage-kv#34) * RO.list: return Key.t instead of string (mirage/mirage-kv#37, fixes mirage/mirage-kv#33) * Introduce a custom error for RW.rename with a source which is a prefix of destination (mirage/mirage-kv#37, fixes mirage/mirage-kv#31) * Use Optint.Int63.t for RO.size, RO.get_partial, RO.set_partial (mirage/mirage-kv#37, fixes mirage/mirage-kv#32) * Remove RW.batch (mirage/mirage-kv#37, fixes mirage/mirage-kv#29 mirage/mirage-kv#36, discussed at the MirageOS meeting in November, and on the mirageos-devel mailing list in January 2022) * Key.pp: escape the entire string, not individual fragments (mirage/mirage-kv#35)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I was recently reviewing code by @dinosaure that implements
Mirage_kv.RW
, and there were some differences between the implementation and the behavior prescribed by the documentation in mirage-kv (see the thread).The documentation is very precise in describing the behavior of
batch
which I think in general is a good thing. However, I think the behavior can be difficult to achieve in many implementations. It also mandates a default retries value of 13 - which may be undesirably high in many implementations. There seems to be a contradiction as well: It says:And then the next line says
I am not sure how to read this, but if concurrent operations do not affect others during a batch then they shouldn't make the batch fail, or?
mirage-kv/src/mirage_kv.mli
Lines 251 to 264 in c66cf9d
I reviewed a couple of implementations found through
opam list --depends-on mirage-kv
:fat-filesystem
,chamelon
,tar-mirage
,irmin-mirage-git
andgit-kv
(which was pinned on my computer)Ocaml-fat only implements
KV_RO
interface despite having write support. I don't know why the fat implementation doesn't implementRW
, but I imagine it would be difficult to implementbatch
as described. Otherwise I think implementingRW
should be easy. https://github.com/mirage/ocaml-fat/blob/6d675256565f9818ca21043e7d7f388a3f4fae36/src/fat.mli#L93-L110Chamelon implements a 'noop'
batch
(i.e. callingbatch kv f
andf kv
is equivalent): https://github.com/yomimono/chamelon/blob/13b08cb7926855a7be8bc9a3787e56395a925506/mirage/kv.ml#L189-L193Tar-mirage implements as well a 'noop'
batch
: https://github.com/mirage/ocaml-tar/blob/main/mirage/tar_mirage.ml#L434Irmin-mirage-git may be closest to implementing the described semantics (I am not sure, I don't fully understand the behavior from a quick read of the code), but notably it defaults to 42 retries instead of 13. I suspect irmin-mirage-git motivated adding
batch
to mirage-kv. https://github.com/mirage/irmin/blob/3dd2231f7d1d7d409ae4704486183d49f79e7095/src/irmin-mirage/git/irmin_mirage_git.ml#L297-L339Since no one seems to implement
batch
exactly as described I think we should consider changing or loosening the requirements of implementations or just removebatch
altogether.The text was updated successfully, but these errors were encountered: