-
Notifications
You must be signed in to change notification settings - Fork 38
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
Byoung/object store async lookup #253
Conversation
I've not reviewed the code, but I thought I leave a comment to let people know that it does in fact work! To run the test JS SDK application, you can use the steps below: The application should now be running on http://127.0.0.1:7676/ Visiting http://127.0.0.1:7676/ should return the word If the above two routes work - then async lookups are working 🥳 |
Great to see this making progress! On have not reviews but want to request that we change all references to object store to kv store, since this will ship after the name change has happened. |
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.
Looks good overall, just had a few comments about names to take into account together with the kv renaming.
Here are the tickets that I think outline all the stuff we should add https://fastly.atlassian.net/browse/EDATA-55 |
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.
Looks great! ✨
Not sure if some of this is overkill or not. Particularly adding
AsyncItem::PendingLookup(PeekableTask<Result<Vec<u8>, ObjectStoreError>>)
rather than converting the object store response to a Response and cramming it inAsyncItem::PendingReq(PeekableTask<Response<Body>>)
.But I thought it'd certainly be possible that some of the attached functionality on the internal side might warrant a different type.
Also @JakeChampion, I made a minor change to the .witx, for
pending_lookup_wait()