[ADDED] JetStream: nats.DirectGet() and nats.DirectGetNext() options

[kozlovic]

The AllowDirect boolean must be set in the stream configuration.
When that is the case, then all servers (leader and replicas) are
part of a DQ group and can respond to a "DIRECT.GET" request.

The KeyValue store makes use of the direct get when connecting
to a stream with the AllowDirect option.

Signed-off-by: Ivan Kozlovic [email protected]

[coveralls]

Coverage increased (+0.4%) to 84.488% when pulling b22f6cb on js_direct_get into f4a86f3 on main.

[derekcollison]

Do we need to get the stream somewhere for this call? If so can we overload instead of introducing DirectGetMsg?

[kozlovic]

Do we need to get the stream somewhere for this call?

Sorry, I am not sure what you mean here.

We could add the DirectGetMsgRequest as a JSOpt (like we do for stream info and purge), but it still would look weird since in GetMsg() "seq" is a mandatory param, so one would have to pass seq==0 if passing a DirectGetMsgRequest option.

[derekcollison]

I think to make the call we have we at least need stream name. I was not sure if we had more such that we could remember the state of directAllowed.

I might opt for here to have direct be a subOpt. KV is where it is critical path, so this not as important IMO.

[kozlovic]

I think to make the call we have we at least need stream name. I was not sure if we had more such that we could remember the state of directAllowed.

I think I understand, but GetMsg() is not looking up the stream, so there is no state to remember if AllowDirect is set or not. The comment in server code was that if one does a "direct get" to a stream that is not set for allow direct, it would get a timeout or no responder.

I might opt for here to have direct be a subOpt.

But then it means something like: GetMsg("myStream", 0, &nats.DirectGetMsgRequest{NextFor: "foo"}). I could remove "Seq" from the option request and say that if it is provided in the GetMsg() call (2nd param) then use this one to construct the request? (would have to have a non exported request struct WITH the sequence for the marshal'ing though). That looks a bit ugly.

KV is where it is critical path, so this not as important IMO.

Well, as you can see, in KV I do call the new API, so if it is GetMsg() with option, I would still need to pass the option to that call, so not sure if that makes any difference. Meaning KV is using the API...

[derekcollison]

We could add new opt, nats.DirectGet, or nats.AllowDirect or something, to make it simpler.

Would you push down that path or just keep GetMsgDirect?

[derekcollison]

The management path is not hot path, so I would say let's try to make an option. Under covers directGet() should exist and KV can use that directly..

[aricart]

On the JavaScript client this is implemented like this:

https://github.com/nats-io/nats.deno/blob/main/nats-base-client/jsmdirect_api.ts

The DirectMsgRequest type is really an union of the possible JSON option combinations - the compiler will reject if the shape of the JSON doesn't line up with one of the types:

export type DirectMsgRequest =
  | SeqMsgRequest
  | LastForMsgRequest
  | NextMsgRequest;

export type NextMsgRequest = {
  seq: number;
  next_by_subj: string;
};

export interface SeqMsgRequest {
  seq: number;
}

export interface LastForMsgRequest {
  "last_by_subj": string;
}

The permutation of the options complicates the API for other languages (like Go), I think instead should have the 3 function that takes the required arguments for the particular combination. That assigns a name for the query they are doing, and helps them out.

[wallyqs]

I think that to avoid adding another API, we could make a new type that implements configureJSContext so that it can be used as a JSOpt, then that way it would be possible to call like this for the direct version:

js.GetMsg("foo", 1, &nats.GetMsgRequest{Direct: true, NextFor: "bar"}) // Direct: false if not direct needed

[kozlovic]

But then for a direct and LastFor, where seq is not supposed to be present, it will be:

js.GetMsg("foo", 0, &nats.GetMsgRequest{Direct: true, LastFor: "bar"})

Notice that user will have to pass 0 for the sequence. If that is ok with everybody, we can do that.

[wallyqs]

Maybe for LastFor we suggest using GetLastMsg instead and only allow &nats.GetMsgRequest{Direct: true} option?

[derekcollison]

Where are we on this, should we re-review?

[kozlovic]

@derekcollison I will update based on @wallyqs recommendations and ping here when it is ready for re-review.

[kozlovic]

@derekcollison @wallyqs Added options. You can review individual commit to see what I changed, or maybe easier to review the total change (Go to Files Changed and make sure that you look at "Changes from all commits").

[derekcollison]

LGTM

[wallyqs]

LGTM

[codegangsta]

🎉

…

Sent from my iPhone

On Jul 28, 2022, at 8:47 PM, Derek Collison ***@***.***> wrote:  @derekcollison approved this pull request. LGTM — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.