-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat(*): major updates to the keyvalue interfaces #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly editorial nits. Great work!
README.md
Outdated
Implementations available for at least Windows, Linux & MacOS. | ||
`wasi:keyvalue` must have at least two complete independent implementations demonstrating | ||
embeddability in a production key-value store context. The implementations must be able to run | ||
on at least two different operating systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we enumerate linux, macOS, Windows. I'd prefer we don't deviate from that unless necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is that, unlike wasi:filesystem
and wasi:sockets
, the underlying OS doesn't really matter for wasi:keyvalue
. Instead, what I think matters quite a lot is that wasi:keyvalue
can be implemented in terms of relevant mainstream key-value servers/databases, including both open-source implementations (say Redis or RocksDB or etcd or ?) and proprietary public cloud implementations (say CosmosDB or DynamoDB). Saying just "two" seems to perhaps set the bar too low, although I'm not sure exactly what the right bar is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that wasi-keyvalue should be able to implement mainstream keyvalue stores. Will add that to the portability criteria.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified the portability criteria to include all three OSes and at least two impl in both open-source and proprietary keyvalue stores. (That means at least 4 impls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this, just some super minor nits I found while reading through
wit/readwrite.wit
Outdated
/// | ||
/// A read/write operation is an operation that acts on a single key-value pair. | ||
/// | ||
/// The value in the key-value pair is defined as a `u8` byte array and the intention |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is added after all the code reviews to clarify why the value in key-value pair is defined as a list<u8>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes all look great to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Could you PTAL? @lukewagner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here! While we probably need more work precisely specifying consistency, this seems like a good starting point for a proposal at this stage. But since names are always harder to change later, I'd like to suggest a few superficial renamings to anticipate future additions; lemme know what you think:
… get-keys This commit has changed many function signatures to account for the scenario where the key does not exist. Instead of returning an error for non-existance keys, the expected behvaior should be returning a none for the client to handle. The other changes are for providing more clarity to the behavior of the APIs ine `wasi:keyvalue` interfaces. It explained what it means to have batch operations without guarantees to atomicity and what are atomic operations like CAS. It also changed the portability criteria to follow the newest WASI proposal template, and added a new goal regarding keyvalue operation performance in this proposal. Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Co-authored-by: David Justice <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
get
, get-many
and get-keys
Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this and think we just need to merge and move forward. One point of dissent I want to record for posterity sake is that I think this interface is the wrong place to be worrying about having different types of consistency guarantees. These are supposed to be for the so-called "80% use case" and I think that just having a basic get/set/del/list along with their batch equivalent should be useful. Elsewhere I've proposed a wasi-cloud-ext
world that could contain stricter things around consistency guarantees for a keyvalue interface
Could you please take another look, @lukewagner? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the consideration! lgtm
This commit has changed many function signatures to account for the scenario where the key does not exist. Instead of returning an error for non-existance keys, the expected behvaior should be returning a none for the client to handle.
The other changes are for providing more clarity to the behavior of the APIs ine
wasi:keyvalue
interfaces. It explained what it means to have batch operations without guarantees to atomicity and what are atomic operations like CAS.It also changed the portability criteria to follow the newest WASI proposal template, and added a new goal regarding keyvalue operation performance in this proposal.
It also changed the
readwrite
andbatch
interface names toeventual
andeventual-batch
to indicate the operations are expected to run in an eventual consistent platform.Closes #24 #25 #9