Exploration: wasi-keyvalue #2486
Conversation
|
I had to add the |
|
Do folks have any insights on how to move this forward? It seems to work but I am not quite sure what do about the worlds stuff:
|
|
Yup, I can take a look tomorrow morning. |
|
@kate-goldenring might also be interested, given her recent work on the specification. |
crates/key-value/src/lib.rs
Outdated
| async fn open( | ||
| &mut self, | ||
| identifier: String, | ||
| ) -> anyhow::Result<Result<Resource<wasi_keyvalue::store::Bucket>, wasi_keyvalue::store::Error>> |
There was a problem hiding this comment.
You can use the trappable_error_type option to make the signatures here a bit easier to work with. See the llm implementation for an example use.
There was a problem hiding this comment.
Thanks @rylev. I gave this a try and am not sure how I feel about. I've left it in a separate commit and would value your thoughts...
crates/key-value/src/lib.rs
Outdated
| ) -> anyhow::Result<Result<wasi_keyvalue::store::KeyResponse, wasi_keyvalue::store::Error>> | ||
| { | ||
| if cursor.unwrap_or_default() != 0 { | ||
| anyhow::bail!("list_keys: cursor not supported"); |
There was a problem hiding this comment.
Is the plan to eventually support the cursor?
There was a problem hiding this comment.
"Plan" is a strong word but, yes, it would be desirable to do on back-ends that support it. For this initial implementation I stuck to using the existing KV implementation which unfortunately do not support cursors.
| @@ -16,6 +16,7 @@ world http-trigger-rc20231018 { | |||
| world platform { | |||
| include wasi:cli/imports@0.2.0; | |||
| import wasi:http/outgoing-handler@0.2.0; | |||
| import wasi:keyvalue/store@0.2.0-draft; | |||
There was a problem hiding this comment.
We definitely need to decide how we want to handle introducing new imports. This will break any Spin runtime that does not have wasi:keyvalue support.
There was a problem hiding this comment.
It seems like we should be bumping the WIT package version here right? This is still a little scary as we haven't even done it with WASI 0.2.1 yet...
There was a problem hiding this comment.
Yes, this was one of my questions: #2486 (comment). Do we need a separate conversation about how to do this?
There was a problem hiding this comment.
Do we need a separate conversation about how to do this?
Probably? I'm not sure that anyone knows how best to handle this yet.
There was a problem hiding this comment.
LGTM overall -- thanks for doing this!
I expect we'll need to implement cursors before we can really claim to support wasi:keyvalue/store, and that may be the most interesting part of this project given the variety of backends we now support (SQLite, Redis, and Azure).
| @@ -30,6 +31,7 @@ world platform { | |||
| world platform-rc20231018 { | |||
| include wasi:cli/reactor@0.2.0-rc-2023-10-18; | |||
| import wasi:http/outgoing-handler@0.2.0-rc-2023-10-18; | |||
| import wasi:keyvalue/store@0.2.0-draft; | |||
There was a problem hiding this comment.
I'd recommend not adding anything new to platform-rc20231018 -- it's only really here for backwards compatibility. In hindsight, I think we probably shouldn't have added mqtt either.
There was a problem hiding this comment.
I agree! The trouble is that the tests all (currently) use this world, so if we want tests for wasi-keyvalue then we will need to rev the test world, and I'm not sure of the implications of that. #2486 (comment)
|
@kate-goldenring I was planning to leave it as draft until we have a decision on WIT versioning. I can mark ready for review and label "do not merge" if that would be better? |
@itowlson leaving as is is fine. Thanks for clarifying the decision criteria. |
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
|
Just to validate an assumption I had reading this PR: this PR adds support for the WASI KV proposal — specifically, it allows guests to use the proposal to interact with stores — and stores are still managed through the manifest Thanks! |
|
@radu-matei Yes. The KV stores are the exact same KV stores as you would get if you used the Spin APIs, specified and configured in exactly the same way. |
An exploratory PR for exploring, apropos of #2447.