nats.go icon indicating copy to clipboard operation
nats.go copied to clipboard

Trial for js.Request

Open bruth opened this issue 3 years ago • 11 comments

I have no intention for this to be merged, but I created a branch so that I could add it to my go.mod for these two experiemental examples request-reply and the actual use case posed in NATS Slack, support for buffering requests in order to auto-scale subscribers (up and down to zero). The link is to the output to observe what it is doing. The code is not well written, but wanted to get something working.

This PR just shows a basic, happy path, implementation of what a js.Request might look. In general, this form of request-reply feels a bit awkward and error prone with one stream. I think a nicer option would be to store replies in a KV that then the requester could fetch using an ID (by default the random inbox token associated.. or maybe a MsgId could be required).

In any case, if nothing else, it is a discussion starter..

bruth avatar Oct 16 '22 14:10 bruth

Coverage Status

Coverage decreased (-0.2%) to 85.763% when pulling e09f13da2bd88ddbaab085085b0885ecd2319197 on js-request-reply into 888a91d4c95ad29f21d0cf74e7964ab6ede7e2fe on main.

coveralls avatar Oct 16 '22 14:10 coveralls

I'm wondering if we want jetstream Response() to have an Ack. I know that it would mean we would not be able to reuse ,msg.Respond, but it's worth considering, knowing request-response is way more async and non-direct in JetStream context.

EDIT: just realized that the response is not a stream. I'm not sure about that approach. I think it should be either fully "async" and not relevant on the other end being online, or fully "sync" (as Core NATS).

Mixed approach might be confusing for the users. Response in many cases could be simply lost without any side knowing about it and being able to react.

Jarema avatar Oct 17 '22 07:10 Jarema

In my mind msg.Respond() should work properly here, detect a header field reply that overrides. Not sure if this is what @bruth did or not.

derekcollison avatar Oct 17 '22 16:10 derekcollison

One thing is we need a deterministic inbox for acks, which possibly needs to be under the $JS.API. So if account A exports JS, the same export/domain should work for the ACKS, otherwise acks execute in the current account's JetStream context.

aricart avatar Oct 17 '22 16:10 aricart

In my mind msg.Respond() should work properly here, detect a header field reply that overrides. Not sure if this is what @bruth did or not.

Yes that is how it works right now. Essentially, it follows the standard nc.Request path of using that multiplexed subscriber to wait for the response (using a NewInbox subject). After a timeout, it will auto-delete the message from the stream (that is another interesting potential design decision/choice depending on the use case).

@Jarema You are right, that in order to support the async replies, we need another stream/KV that will auto-delete ask replies are delivered. IMO as an end-user experience it would be most straightforward if that was an internally managed stream.

One thing is we need a deterministic inbox for acks, which possibly needs to be under the $JS.API.

As it is implemented, the acks shouldn't be affected (server back to publisher, subscriber to consumer/server). The only addition is the subscribe to the inbox and the respond from the subscribe to that inbox subject.

bruth avatar Oct 17 '22 17:10 bruth

@bruth yes, we just need to keep it in the radar that cross-account acks are currently difficult and we need a strategy for exporting that as a service from the source account

aricart avatar Oct 17 '22 17:10 aricart

Why do you think cross account acks are hard? You have to do multiple service exports but I think they are well understood, and hopefully Helix can provide an easy DX with single click.

derekcollison avatar Oct 17 '22 17:10 derekcollison

The $JS.ACK.<stream>.<consumer>.> is not subject to the domain work that we had done. The proposed $JS.ACK.<domain>.<accounthash>.<stream>.<consumer> would allow fixing that.

aricart avatar Oct 17 '22 17:10 aricart

Ah.. now this is connecting the dots. I was going through the ADRs a while back and added support in Python (and have an open docs PR), but he noted, that was not yet implemented in the server?

bruth avatar Oct 17 '22 18:10 bruth

Ah.. now this is connecting the dots. I was going through the ADRs a while back and added support in Python (and have an open docs PR), but he noted, that was not yet implemented in the server?

The clients future proofed some on the handling for the parsing of the JS.ACK

aricart avatar Oct 17 '22 18:10 aricart

Got it.. and by "he", I meant @wallyqs of course.

bruth avatar Oct 17 '22 19:10 bruth