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

Object store iface #165

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Object store iface #165

merged 1 commit into from
Apr 7, 2022

Conversation

lann
Copy link
Collaborator

@lann lann commented Mar 14, 2022

re: #48

@lann lann force-pushed the object-store-iface branch 3 times, most recently from 262d8a6 to 5a7a618 Compare March 14, 2022 20:46
wit/ephemeral/spin-object-store.wit Outdated Show resolved Hide resolved
wit/ephemeral/spin-object-store.wit Outdated Show resolved Hide resolved
wit/ephemeral/spin-object-store.wit Outdated Show resolved Hide resolved
@lann lann force-pushed the object-store-iface branch 2 times, most recently from 75a4af1 to 175f3dc Compare March 15, 2022 13:44
@radu-matei radu-matei mentioned this pull request Mar 17, 2022
8 tasks
@lann
Copy link
Collaborator Author

lann commented Mar 23, 2022

This needs tests but I'd like feedback too.

}

/// Prepare an object with the given key to be written. See `object-writer`.
get-object: function(key: string) -> expected<object-reader, error>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to have a store resource rather than putting the object accessors directly on the storage interface? I know we are skipping multiple store implementations on day one but it would be good to have the design aligned to the eventual plan so as to minimise change and avoid baking in assumptions.

src/commands/up.rs Outdated Show resolved Hide resolved
crates/engine/src/lib.rs Outdated Show resolved Hide resolved
crates/object-store/src/file.rs Outdated Show resolved Hide resolved
crates/engine/src/lib.rs Outdated Show resolved Hide resolved
use crate::wit::spin_object_store;
use cap_std::{ambient_authority, fs::OpenOptions};

pub type FileObjectStoreData = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get the different roles of FOS and FOSData - are clearer names available or is this an artefact of the bindgen stuff? (I vaguely recall being puzzled by something similar in HTTP so maybe the latter?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly an artifact of wit-bindgen, though we could organize them better.

crates/object-store/src/file.rs Outdated Show resolved Hide resolved
let size = file.read(buf).map_err(|err| err.to_string())?;
u64::try_from(size).map_err(|err| err.to_string())
}
None => Err("file closed".to_string()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context or anything else to help with diagnostics, or does that belong at a higher level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above for this specific error, but for errors in general they are being passed to the guest so have to pass through the wasm barrier. I went for string errors for simplicity.

crates/object-store/src/file.rs Outdated Show resolved Hide resolved
wit/ephemeral/spin-object-store.wit Show resolved Hide resolved
@lann lann marked this pull request as ready for review March 29, 2022 16:34
@lann lann force-pushed the object-store-iface branch 3 times, most recently from d1498a6 to 5edb438 Compare March 29, 2022 16:59
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to playing around with this, thanks!

I think this needs a rebase, but LGTM.

- Includes a simple file-based implementation.
@lann lann merged commit 4923a60 into main Apr 7, 2022
@lann lann deleted the object-store-iface branch May 5, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants