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
[ADDED] JetStream: nats.DirectGet() and nats.DirectGetNext() options #1020
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need to get the stream somewhere for this call? If so can we overload instead of introducing DirectGetMsg?
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.
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.
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.
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.
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.
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.
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.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...
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.
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?
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.
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..
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.
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: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.
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.
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: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.
But then for a direct and LastFor, where seq is not supposed to be present, it will be:
Notice that user will have to pass 0 for the sequence. If that is ok with everybody, we can do that.
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.
Maybe for
LastFor
we suggest usingGetLastMsg
instead and only allow&nats.GetMsgRequest{Direct: true}
option?