Conversation
dicej
left a comment
dicej
left a comment
There was a problem hiding this comment.
Looks reasonable to me, especially if we can add some automated test coverage.
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
async-redis-trigger
branch
from
0b588da to
1655e70
Compare
|
Added a (fairly simple) test. |
|
|
||
| /// The full world of a guest targeting a redis-trigger | ||
| world redis-trigger { | ||
| include platform; |
There was a problem hiding this comment.
Just a quick note that this one line is super valuable. As I work on updating spin-go-sdk to work with modern Spin, I noticed that the existing fermyon:spin/redis-trigger world is so ancient it doesn't support any wasi:http imports (or wasi:$anything, for that matter), which makes supporting it in SDKs awkward, since we can't give the user access to e.g. outbound http except via the old fermyon:spin/http interface.
There was a problem hiding this comment.
The WIT worlds are kind of untrue: we link everything unconditinally in the factors. My guess is that if a v1 Redis trigger used WASI P2 HTTP then it would work despite what the WIT world says...
| use spin_trigger::{cli::NoCliArgs, App, Trigger, TriggerApp}; | ||
| use spin_world::exports::fermyon::spin::inbound_redis; | ||
| use spin_world::exports::fermyon::spin::inbound_redis as v1; | ||
| use spin_world::exports::spin::redis::inbound_redis as v3; |
There was a problem hiding this comment.
Per my other comment in this review, we should make sure we've added all the other spin:up/platform stuff to the linker, not just the old fermyon:spin/platform stuff. I don't think there's an easy way to do that only for the V3 case, so we'd be giving V1 components access to all that stuff, too, but that's not a huge problem, I'd imagine.
There was a problem hiding this comment.
I believe RuntimeFactors (via the base trigger implementation) takes care of that. My test shows we have access to async KV (a very recent addition) and I didn't need to do anything to make that work.
There was a problem hiding this comment.
Ah, I just realized the error I was getting was from componentize-go rather than Spin, meaning you can ignore what I said above. Sorry for the noise!
2 participants
This was too simple. It must be a trap.
(Well I say it was too simple, but it still took muggins here long enough to get all the bits to line up.)
Tested manually, initially with a SDK
[redis_component]-based thingy to make sure the existing sync path still worked, and then with a wit-bindgen export binding, and calling async KV operations to make sure that async was working as expected. I haven't clobbered the concurrency side of things though, or error handling really.@alexcrichton @dicej does this look as you would expect, or are there a bunch of subtleties that I should have been considering?