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

API Review - Expand Request Extension Methods #75

Closed
caleblloyd opened this issue Jul 4, 2023 · 4 comments · Fixed by #78
Closed

API Review - Expand Request Extension Methods #75

caleblloyd opened this issue Jul 4, 2023 · 4 comments · Fixed by #78
Assignees

Comments

@caleblloyd
Copy link
Collaborator

After reviewing the Request extension method, I think it might make sense to expand it to more closely match the core API

The idea here is to re-use PubOpts as requestOpts and SubOpts as replyOpts, which means that any future additions to those options will flow through and be fully supported in Request/Reply.

We are actually very close to being able to implement most of the RequestMany options discussed in ADR-40 as well - so I've included an example of how the RequestMany methods could look too

    // RequestAsync methods
    // Same as PublishAsync with the following changes
    // - Response is 0 (null) or 1 NatsMsg
    // - PubOpts is called requestOpts
    // - add SubOpts replyOpts
    //   - default replyOpts.MaxMsgs to 1
    //   - if replyOpts.Timeout == null then set to NatsOptions.RequestTimeout
    
    ValueTask<NatsMsg?> RequestAsync(
        string subject,
        ReadOnlySequence<byte> payload = default,
        in NatsPubOpts? requestOpts = default,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);

    ValueTask<NatsMsg?> RequestAsync(
        in NatsMsg msg,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
    
    ValueTask<NatsMsg<TReply>?> RequestAsync<TRequest, TReply>(
        string subject,
        TRequest data,
        in NatsPubOpts? requestOpts = default,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
    
    ValueTask<NatsMsg<TReply>?> RequestAsync<TRequest, TReply>(
        in NatsMsg<TRequest> msg,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);

    
    // RequestManyAsync methods
    // Same as PublishAsync with the following changes
    // - Response is IAsyncEnumerable<NatsMsg>
    // - PubOpts is called requestOpts
    // - add SubOpts replyOpts
    //   - if replyOpts.Timeout == null then set to NatsOptions.RequestTimeout
    
    ValueTask<IAsyncEnumerable<NatsMsg>> RequestManyAsync(
        string subject,
        ReadOnlySequence<byte> payload = default,
        in NatsPubOpts? requestOpts = default,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
    
    ValueTask<IAsyncEnumerable<NatsMsg>> RequestManyAsync(
        in NatsMsg msg,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
    
    ValueTask<IAsyncEnumerable<NatsMsg<TReply>>> RequestManyAsync<TRequest, TReply>(
        string subject,
        TRequest data,
        in NatsPubOpts? requestOpts = default,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
    
    ValueTask<IAsyncEnumerable<NatsMsg<TReply>>> RequestManyAsync<TRequest, TReply>(
        in NatsMsg<TRequest> msg,
        in NatsSubOpts? replyOpts = default,
        CancellationToken cancellationToken = default);
@mtmk
Copy link
Collaborator

mtmk commented Jul 4, 2023

About options. PubOpts.ReplyTo will have to be overwritten also remember subscription in request isn't a real subscription but a distribution point for the inbox. Creating request options instead would make more sense.

@mtmk
Copy link
Collaborator

mtmk commented Jul 4, 2023

Should we back RequestMany with a return type e.g. NatsReq? Then, similar to NatsSub, we can provide a channel and give the developer cancel the request based on other factors (other than timeouts or counts)

@caleblloyd
Copy link
Collaborator Author

Correct - PubOpts and SubOpts are both a little leaky in the example

  • PubOpts.ReplyTo is always overwritten, or NatsMsg.ReplyTo is overwritten in the case of passing NatsMsg
  • SubOpts.MaxMsgs is always overwritten to 1 in the ReplyAsync methods
  • SubOpts.IdleTimeout has no effect in the ReplyAsync methods

There are a couple ways I can think of for handling this:

  1. Document in the respective method that a particular PubOpt/SubOpt setting is overwritten
  2. Create new options that take these into account, example:
  • RequestOpts - PubOpts without the Reply Inbox
  • SubOneOpts - SubOpts without the options that control timeouts when only one message is desired

@caleblloyd
Copy link
Collaborator Author

Should we back RequestMany with a return type e.g. NatsReq? Then, similar to NatsSub, we can provide a channel and give the developer cancel the request based on other factors (other than timeouts or counts)

I can see that being useful, but should we just be returning the NatsSub at that point?

@mtmk mtmk self-assigned this Jul 5, 2023
@mtmk mtmk linked a pull request Jul 5, 2023 that will close this issue
@mtmk mtmk mentioned this issue Jul 5, 2023
@mtmk mtmk closed this as completed in #78 Jul 6, 2023
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 a pull request may close this issue.

2 participants