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

batch::get-many should parallel store.get #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devigned
Copy link
Collaborator

@devigned devigned commented Nov 4, 2024

get: func(key: string) -> result<option<list<u8>>, error>;

in store returns an option<list<u8>>, but

get-many: func(...) -> result<list<option<tuple<string, list<u8>>>>, error>;)

in batch returns list<option<tuple<string, list<u8>>>>. I would expect get-many to return result<list<tuple<string, option<list<u8>>>>, error>;.

Signed-off-by: David Justice <[email protected]>
Copy link
Collaborator

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

this makes sense to me.

Since this updates the APIs, can you bump the draft version up?

Comment on lines 29 to +32
/// MAY show an out-of-date value if there are concurrent writes to the store.
///
/// If any other error occurs, it returns an `Err(error)`.
get-many: func(bucket: borrow<bucket>, keys: list<string>) -> result<list<option<tuple<string, list<u8>>>>, error>;
get-many: func(bucket: borrow<bucket>, keys: list<string>) -> result<list<tuple<string, option<list<u8>>>>, error>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please update the docs for get-many?

/// get-many returns the key-value pairs associated with the keys in the store. It returns a list of
/// key-value pairs.
/// The returned list of key-value pairs must have the same length as that of the `keys` parameter.
/// If any of the keys do not exist in the store, it returns a (key, `none`) value for that pair in the list.

@thomastaylor312
Copy link
Collaborator

I can go either way on this. I think we may have had it the way you have it in this PR before, but I swapped it out because you have to return a copy of the key. For a batch request, that means even if you have no data to show, you have to return a new key (which could be quite large for some people). Since batch requests are big, this is where extra allocations of keys could matter. I swapped it to the option because it should be returned in the same order as passed in, so if you absolutely need to know which key you need, you can refer to that. Here are our options and the tradeoffs:

What you have in this PR:

list<tuple<string, option<list<u8>>>>

Advantages: Easiest to consume because you have each KV pair
Disadvantages: Extra allocation of every key that is passed in

Current:

list<option<tuple<string, list<u8>>>>

Advantages: No extra allocations of non-existent keys
Disadvantages: Awkward mapping back to initial list of keys if you need to know the missing key

Option 3

list<option<list<u8>>>

Advantages: No extra allocation of keys
Disadvantages: Required iterate over the list of keys and results together (which can be awkward/annoying in some languages)

So I think those are the options. After putting some thought into it here, I'd rather have option 3 to avoid all sorts of extra memory usage (and copying), but we should decide together what we'd prefer here

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