-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add JetStream NATS Client Extensions #598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very minor stuff (If it wasn't handled before I finally submitted >_<)
/// <param name="pending">The number of pending messages at the time the drop occurred.</param> | ||
/// <param name="msg">The dropped message represented by <see cref="NatsMsg{T}"/>.</param> | ||
/// <typeparam name="T">Specifies the type of data in the dropped message.</typeparam> | ||
void OnMessageDropped<T>(NatsSubBase natsSub, int pending, NatsMsg<T> msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had one 'thought' here...
maybe as <remarks>
XmlDoc... explaining to people why this -isn't- a valuetask... e.x.
<remarks>This method is expected to complete quickly to avoid further delays in processing; if complex work is required, it is recommended to offload to a channel or other out-of-band processor</remarks>
but... as usual may be overthinking >_<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is an important detail to mention
@@ -35,7 +35,7 @@ protected override void TryComplete() | |||
} | |||
} | |||
|
|||
internal class InboxSubBuilder : ISubscriptionManager | |||
internal class InboxSubBuilder : INatsSubscriptionManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost left a comment here but see that ISubscriptionManager
was internal so it shouldn't break things for sane consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. I guess it will break binary compatibility but we're coming out with a few breaking changes here applications will need to rebuild anyway.
public interface INatsSubscriptionManager | ||
{ | ||
public ValueTask RemoveAsync(NatsSubBase sub); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is public now should we XMLDoc it?
(If 'public but really internal' perhaps an XMLDoc stating it is an internal API subject to change?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hi, As part of this PR would it please be possible to expose the INatsConnection Connection property from NatsJSContext, either by making it Public, or a function like In the same way, could NatsKVStore and NatsObjStore expose the underlying NatsJSContext? This would be really handy as it would allow developers to easily add their own extension methods if need be. Would be great if this was possible! Many thanks! |
Good idea! done as a property.
We have a few issues as todo items for those, added a note to do that:
thanks @darkwatchuk 💯 |
That's great - thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work.
thanks @rickdotnet 💯 I will merge just after 2.4 stable release which I'm planning to do first thing next week |
* Add placement configuration to key-value and object store (#650) * Bump System.Text.Json from 8.0.4 to 8.0.5 in /src/NATS.Client.Core (#649) * Add other client extensions (#637) * Add Domain support for stream mirroring and sourcing and KV full support for the same. (#631) * Add type-forwarders NET 5.0+ (#641) * Add JetStream NATS Client Extensions (#598)
* Add placement configuration to key-value and object store (#650) * Bump System.Text.Json from 8.0.4 to 8.0.5 in /src/NATS.Client.Core (#649) * Add other client extensions (#637) * Add Domain support for stream mirroring and sourcing and KV full support for the same. (#631) * Add type-forwarders NET 5.0+ (#641) * Add JetStream NATS Client Extensions (#598)
This PR introduces an interface-based Core API dependency for the JetStream API and an extension to
INatsClient
interface:As a result of this change, several internal APIs will need to be made public. This will enable application developers to extend some of the lower-level features. This aligns with our overall mission to establish
INatsClient
as the default API level, providing the opportunity to introduce lower-level functions from our library throughINatsConnection
.INatsConnection
: Opened functions mostly useful for creating custom subscribers which are used in JetStream consumer implementation.NatsSubBase
: Reviewed and documented subscription base class to be inherited for custom subscriptions.INatsSubscriptionManager
: Exposed the subscription manager as an interface, only having the single function opened used in current implementations. This could be opened up more as we see opportunities or demand.JetStream
NatsClientExtensions
: Convenience extension to create JetStream context.