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

JetStream consumer options #107

Merged
merged 14 commits into from
Aug 9, 2023
Merged

JetStream consumer options #107

merged 14 commits into from
Aug 9, 2023

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Aug 2, 2023

Implement JetStream Consumer API consume() options based on ADR-37.

The design idea is to implement a NatsJSSubBase class to handle control messages and timeouts. Then we can implement specific classes for all three calls, next(), fetch() and consume(). In this PR only consume() is implemented.

@mtmk
Copy link
Collaborator Author

mtmk commented Aug 8, 2023

We seem to have an issue with NATS.Client.Core.Tests.NatsConnectionTestTls.UserCredentialAuthTest against nats-server dev version. Can't repro on my machine.

@mtmk mtmk changed the title [WIP] Jetstream consumer opts JetStream consumer options Aug 8, 2023
@mtmk mtmk marked this pull request as ready for review August 8, 2023 20:57
@mtmk mtmk requested a review from caleblloyd August 8, 2023 20:57
Comment on lines 78 to 82
_userMessageChannel = Channel.CreateBounded<NatsJSMsg<T?>>(new BoundedChannelOptions(1_000)
{
FullMode = BoundedChannelFullMode.Wait,
AllowSynchronousContinuations = false,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this take the options from NatsSubOpts.ChannelOptions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True 💯 I didn't think of that.

Comment on lines 87 to 91
_notificationChannel = Channel.CreateBounded<NatsJSNotification>(new BoundedChannelOptions(1_000)
{
FullMode = BoundedChannelFullMode.DropOldest,
AllowSynchronousContinuations = false,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

1k seems excessive but I don't really know what a good number would be - 10? 100? 😆

Also doesn't really seem worth making configurable. I guess they are small so 1k might be fine.

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 920a3ef into main Aug 9, 2023
6 checks passed
@mtmk mtmk deleted the jetstream-consumer-opts branch August 9, 2023 16:43
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