JetStream Ordered consumer #169
Merged
Conversation
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
Simplified consumer api to just consume, fetch and next methods removing the *All* versions. Now consume and fetch methods return IAsyncEnumerable only. This reduces the API foot-print as well as making it consistent between 'Ordered Consumer' and 'Consumer'. * Removed consume and fetch methods exposing channel reader. * Renamed *All* methods e.g. FetchAllAsync() to FetchAsync(). * Also removed the interfaces exposing the channel readers.
# Conflicts: # tests/NATS.Client.JetStream.Tests/ConsumerFetchTest.cs # tests/NATS.Client.JetStream.Tests/JetStreamTest.cs
|
What can we do about the disparity with core subscribe? Subscribe call on core on the other hand still returns an interface public interface INatsSub<T> : IAsyncDisposable
{
ChannelReader<NatsMsg<T>> Msgs { get; }
string Subject { get; }
string? QueueGroup { get; }
public ValueTask UnsubscribeAsync();
}An idea here maybe to introduce a different call to match JetStream calls: // New matching call
IAsyncEnumerable<NatsMsg<T>> SubscribeAsync<T>(string subject, ...);
// Existing call renamed
ValueTask<INatsSub<T>> SubscribeCoreAsync<T>(string subject, ...); |
caleblloyd
reviewed
Nov 6, 2023
| if (!string.IsNullOrWhiteSpace(request.Config.Name)) | ||
| { | ||
| subject += $".{request.Config.Name}"; | ||
| request.Config.Name = default!; |
There was a problem hiding this comment.
spot on! no need for it.
Comment on lines
195
to
199
| if (!string.IsNullOrWhiteSpace(request.Config.FilterSubject)) | ||
| { | ||
| subject += $".{request.Config.FilterSubject}"; | ||
| } | ||
|
|
There was a problem hiding this comment.
What if the name was not set, but the filter subject was set? With this logic the subject token for name is not present, and the filter subject token is appended to where the name subject should go
There was a problem hiding this comment.
FilterSubject is never set actually. Dead code. I think it might be my copy & paste error. Sorry about that!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ordered consumer implementation based on https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-17.md
API change discussion:
Simplified consumer api to just consume, fetch and next methods removing the
*All*versions. Now consume and fetch methods (renamed from the*All*versions) returnIAsyncEnumerableonly. This reduces the API footprint as well as making it consistent between 'Ordered Consumer' and 'Consumer'.*All*methods e.g.FetchAllAsync()toFetchAsync().resolves #32