-
Notifications
You must be signed in to change notification settings - Fork 9
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
Async object store support #6
Conversation
This looks interesting! Any chance to get this merged and released? @roeap |
Any progress getting this released? |
This comment has been minimized.
This comment has been minimized.
I have picked it up. |
Maybe it's easier to fork, and then merge all open PRs into that fork and release it |
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 taking care of this @kylebarron! And sorry for letting this sit for so long.
Change list
PyBytes::new(py, &Bytes::to_vec())
had two clones, I believe, where only one was needed.PyBytes
fromCow<[u8]>
instead of converting manually. At least for the async version this was a lot easier; I struggled with manually constructingPyBytes
objects inside the async closure.Another option for the API would be to have a separate class
AsyncObjectStore
that has only the async methods. This might keep the methods cleaner (we wouldn't need to have anasync
suffix on each method). But alternatively, there's no reason why someone couldn't use the same store for both synchronous and async calls.This is implemented on top of #5, so ignore the first several commits. It's expected for #5 to be merged first, and then this PR can be squashed.
Closes #2