Skip to content
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

[FEAT] [EXPERIMENTAL] added support for next_by_subj to direct stream api #325

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

aricart
Copy link
Member

@aricart aricart commented Jul 7, 2022

[REFACTOR] moved direct API to its own JSM api class

[REFACTOR] moved direct API to its own JSM api class
@aricart aricart requested a review from kozlovic July 7, 2022 15:21
@aricart aricart temporarily deployed to CI July 7, 2022 15:36 Inactive
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my latest comment in the ADR regarding what needs to be returned to the user and question about "NATS" direct get specific headers..

}

get lastSequence(): number {
const v = this.header.get(RepublishedHeaders.JsLastSequence);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing that being set by the server. Where would that come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good catch - this was related to the RepublishedMessageHeaders which is not what this PR is about.

…ce to `Nats-Last-Sequence` as this is a Republished Message Header.
@aricart aricart temporarily deployed to CI July 7, 2022 18:16 Inactive
@aricart aricart requested a review from kozlovic July 7, 2022 18:17
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confused on what DirectMsgRequest is, since I don't see how this is used. Also looks like NextMsgRequest is missing a field? (but again, not sure how this is being used).

*/
export type NextMsgRequest = {
seq: number;
next_by_subj: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that NextMsgRequest should also have last_by_subj, which corresponds to the enum below to "LastForMsgRequest"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, the argument that you can provide for getMessage as a DirectMsgRequest which is an alias for one of three types LastForMsgRequest is one of the possible values:

export type DirectMsgRequest =
  | SeqMsgRequest
  | LastForMsgRequest
  | NextMsgRequest;

next_by_subj: string;
};

export type DirectMsgRequest =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this is for though... looking at the example below, you are creating the request manually and pass last_by_subj (which is missing in the exported NextMsgRequest object above)..

Copy link
Member Author

@aricart aricart Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API returns a StoredMsg which is an interface (same as what you can $JS.API.STREAM.MSG.GET.${stream}. In TypeScript this is typed, as you cannot ack or do anything with these messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(typescript is just looking for a type that has the fields defined by the type) it is not cast/it is just something that has that 'shape'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We followed up on a call and it is clear to me now.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aricart aricart merged commit e17fb3d into main Jul 7, 2022
@aricart aricart deleted the direct-next branch July 7, 2022 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants