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

Revised request API #78

Merged
merged 3 commits into from
Jul 6, 2023
Merged

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Jul 5, 2023

Revised request(many) API based on #75
Also added start-up timer as per ADR-40. It's the max time to wait for the first message.

Also added start-up timer as per ADR-40. It's the
max time to wait for the first message.
@mtmk mtmk requested a review from caleblloyd July 5, 2023 13:33
@mtmk mtmk linked an issue Jul 5, 2023 that may be closed by this pull request
@mtmk
Copy link
Collaborator Author

mtmk commented Jul 5, 2023

Are we happy with the public interface i.e. NatsSub and Request*Async?

  • I had doubts about the options being passed to Request but I can see the value of not littering the API with too many types.

  • RequestMany returning async enumerator forces the developer to use await foreach. Returning a type with a channel e.g. NatsSub may be a better option.

@caleblloyd
Copy link
Collaborator

caleblloyd commented Jul 5, 2023

For the Request options - we need a way to pass Queue Group and Headers when not using Model types. So the only thing missing from NatsPubOpts is the ReplySubject which is defaulted.

For the Reply options of the RequestAsync methods we could cut those down, as many of the timeouts do not apply when waiting for a single reply. Or we could document that they have no effect when waiting for a single reply.

For the Reply options of RequestManyAsync - this pretty much does need to be the full set of NatsSubOpts

I think that returning NatsSub is fine for the RequestManyAsync methods

this NatsConnection nats,
string subject,
ReadOnlySequence<byte> payload = default,
NatsPubOpts? requestOpts = default, // Not used
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be used - Headers and Queue Group could be set here right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, spot on! Thank you. I forgot about headers when publishing. Queue group doesn't apply to pub-opts. But a good question. Does it make sense to have a queue group on an inbox subscription? If it does, how should we go about it when our inbox is mux-ed and shared, as in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops - yes - queue group is on subscribe. Good catch. No, I don't think an Inbox Subscription really needs to support a Queue Group, but it's also not a big deal if it does

@mtmk
Copy link
Collaborator Author

mtmk commented Jul 5, 2023

For the Request options - we need a way to pass Queue Group and Headers when not using Model types. So the only thing missing from NatsPubOpts is the ReplySubject which is defaulted.

see comment above

For the Reply options of the RequestAsync methods we could cut those down, as many of the timeouts do not apply when waiting for a single reply. Or we could document that they have no effect when waiting for a single reply.

I'm happy with documenting it. Changed my mind a little. Maybe it's better to keep Type footprint to a minimum. Also, in my opinion at this level of the API it's better to give clues that under the bonnet it is still a pub/sub powering the request reply pattern. We can certainly wrap this later with a much more opinionated/easy/lite API for less demanding applications.

For the Reply options of RequestManyAsync - this pretty much does need to be the full set of NatsSubOpts

Agreed.

I think that returning NatsSub is fine for the RequestManyAsync methods

I was thinking to wrap this PR up as a smaller step and discuss this on a some kind of 'internal redesign' PR. Happy to carry on here though if you like.

@caleblloyd
Copy link
Collaborator

I was thinking to wrap this PR up as a smaller step and discuss this on a some kind of 'internal redesign' PR

That sounds good!

@mtmk mtmk marked this pull request as ready for review July 6, 2023 08:50
@mtmk mtmk requested a review from caleblloyd July 6, 2023 08:51
Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit ed6c255 into main Jul 6, 2023
2 checks passed
@mtmk mtmk deleted the 75-api-review-expand-request-extension-methods branch July 6, 2023 13:10
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.

API Review - Expand Request Extension Methods
2 participants