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

New RO and RW signatures #14

Merged
merged 14 commits into from Feb 24, 2019

Conversation

Projects
None yet
2 participants
@samoht
Copy link
Member

samoht commented Oct 4, 2018

/cc @linse and @hannesm

The RO interfaces is a bit more complex now, but I think it's better / simpler to use / more useful. I am a bit undecided about exposing size, mtime and digest in that interface, but why not.

@hannesm hannesm referenced this pull request Oct 5, 2018

Merged

Switch to dune #13

@samoht samoht force-pushed the samoht:rw branch 3 times, most recently from f3d5705 to 0efccf5 Oct 5, 2018

@samoht samoht force-pushed the samoht:rw branch 2 times, most recently from 2dddafb to 40ef778 Oct 6, 2018

@@ -55,9 +55,9 @@ module type RO = sig
val pp_error: error Fmt.t
include Mirage_device.S
type value
val exists: t -> key -> [`Value | `Dictionary] option io
val exists: t -> key -> ([`Value | `Dictionary], error) result io

This comment has been minimized.

@samoht

samoht Oct 6, 2018

Author Member

@linse @hannesm I am very not sure about that change; one would expect exists to return some kind of true/false answer, and now it's either [Value | Dictionary]or aNot_founderror. I think I prefer to have([Value | Dictionary] option, error) result ioin that case, to signal thatNone` means "doesn't exist" but in that case the error could only be stuff private to the implementation (e.g. we can never return any of the the known variants).

WDYT?

end

type key = Key.t
(** The type for keys. *)

This comment has been minimized.

@hannesm

hannesm Oct 20, 2018

Member

I'd remove this alias here, since fd4aac1 RO contains this type alias. (only used below in type error, I'd replace occurrences with Key.t there)

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Oct 20, 2018

LGTM

@hannesm
Copy link
Member

hannesm left a comment

@linse and @hannesm looked at the pull request in detail, which looks fine (we should get this merged ASAP)

we've been thinking a bit more about KV and FS abstractions.
In our opinion, KV is a persistent Map (and should comply with the Map interface) with key being either flat or structured.
A FS interface should extend KV, and map each key to a pair of value and properties (another map).

we're interested in feedback :)

@@ -22,36 +22,201 @@

(** {1 Mirage_kv} *)

type error = [`Unknown_key of string]
(** MirageOS key-value stores are nested dictionaries, associating
structured {{!Key}keys} to either dictionaries or values. *)

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

what is the definition of structured and why is it relevant here?

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

why do we have nested dictionaries?

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

include some information whether this library also works when we have no nested dictionaries (flat kv store)? does it perform well? do we recommend something else?

Ultimately, should we abstract (functorize?) over type key / module Key in mirage-kv? This allows a user to select their key (e.g. a string, or a path, or a custom variant type), and implementations to use the module Key interface. The path key implemented here by module Key, a flat string key should be provided by mirage-kv. is using a string as key a special case of the path (using a single segment path) -- or does this imply computational overhead (need benchmarks!)?

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

mention that the V of KV is always a string. we should write that explicitly.

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

should a KV implement the Map.S interface? apart from IO/persistency, and that a Map is polymorphic in its value (whereas our KV always uses string for values), are there any other differences?

(** [empty] is the empty key. It refers to the top-level
dictionary. *)

val v : string -> t

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

why v? value? this confuses me in the scope of a key-value interface, where the key has a function named value.

maybe of_string is a better name

(** MirageOS key-value stores are nested dictionaries, associating
structured {{!Key}keys} to either dictionaries or values. *)

module Key: sig

This comment has been minimized.

@hannesm

hannesm Jan 7, 2019

Member

NB: this implements keys as path. based on the remarks and decision above, we should implement an interface Key with two implementations, Flat_key and Path_key - and fit the functions below into either module (depending on mirage-kv implementors need). Initially Flat_key could be implemented by a single segment Path_key.

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Jan 7, 2019

as discussed in paris, we prepared a mirage-dev overlay with these changes at https://github.com/mirage/mirage-dev/tree/kv-ng (irmin{-mirage} does not compile yet, mirage-fs-lwt only contains a stub implementation (assert false) to satisfy this interface).

@hannesm hannesm force-pushed the samoht:rw branch from ecbb394 to 2a5f91e Feb 24, 2019

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Feb 24, 2019

I rebased on master, and constrained existing opam packages to mirage-kv{-lwt} < 2.0.0 in ocaml/opam-repository#13497. once CI passes here, I'll merge and release (and copy the comments into separate issues to not loose them)

@hannesm hannesm merged commit c4720f6 into mirage:master Feb 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.