Open thomastaylor312 opened 3 months ago
Thanks for all the great feedback! Gimme until tomorrow and I should be able to make all the changes
Ok I've pushed up some additional changes as discussed
My plan was to actually implement something with this interface and then copy over. I deleted purely because it was completely out of date. I am also fine to put something back together once people are ok with the changes. I'll do that before we merge
Hey all! Sorry for the delay in responding, got pulled away to some other work. I've implemented all suggested changes and I think we're ready for a final review @danbugs @devigned @lukewagner. Once we've approved this, I'll add back an examples file that matches the new interfaces before merging
Please note that I just pushed one more commit (so we can pop it off if people hate it) to change how request-multi
works. One of the uses of request-multi
is to support a scatter/gather operation. In these cases, you might not know how many requests you are going to receive, so you can't set expected replies. Generally these wait until timeout and then return the results. This commit adds the ability to support all the different use cases for request-multi by adding expected replies to the request options. We don't have to support this, but when I was reviewing different use cases one more time, I was reminded of the scatter/gather operation. Just let me know if people hate it or if it isn't documented clearly
@lukewagner and I had a good convo about the reply stuff today as well as what the portability criteria should be. I am going to noodle on that a bit and also probably get some feedback from the Wasm WG in CNCF (since they've been interested in this) on the possible options. So I might wait to review at least the request/reply stuff until we can have some conversations
Please note that I just pushed one more commit (so we can pop it off if people hate it) to change how
request-multi
works. One of the uses ofrequest-multi
is to support a scatter/gather operation. In these cases, you might not know how many requests you are going to receive, so you can't set expected replies. Generally these wait until timeout and then return the results. This commit adds the ability to support all the different use cases for request-multi by adding expected replies to the request options. We don't have to support this, but when I was reviewing different use cases one more time, I was reminded of the scatter/gather operation. Just let me know if people hate it or if it isn't documented clearly
Btw - I am pretty sure that some implementors (like, maybe, Apache Kafka) do not inherently support a singular request, and, instead, often operate w/ returning a batch of messages as they are designed for high throughput. I think it's possible that you can still hack together a singular request implementation, but it's worth keeping in mind that that is a anti-pattern for some implementors.
@danbugs So the more I look at this, the more I think having request and request-multi is what causes the problem here. As it currently stands a request-multi
with expected replies set to 1 is the exact same as request
, so why not just simplify it to one thing. It also avoids any problems with breaking interfaces in minor versions and any issues with implementations like Kafka
@danbugs So the more I look at this, the more I think having request and request-multi is what causes the problem here. As it currently stands a
request-multi
with expected replies set to 1 is the exact same asrequest
, so why not just simplify it to one thing. It also avoids any problems with breaking interfaces in minor versions and any issues with implementations like Kafka
I am good with unifying request-multi
and request
👍
I prefer if the spec works for n
most of the time instead of 1
or n
This PR incorporates various points of feedback from discussion amongst the interface champions, users of Wasm projects, and discussion in the CNCF wasm WG. This smooths over some of the rough edges and adds support for request/reply paradigms. Some of the bigger changes are:
format
type in favor of an optional content type stringtopic
field to themessage
type