-
Notifications
You must be signed in to change notification settings - Fork 815
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
Use recommended NATS.Net client for Nats package #2336
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.
The changes look good, I've added some ideas for minor improvements. Thank you @Alirexaa !
BTW is there any chance you could find some time to add a README.md
and use NATS.Extensions.Microsoft.DependencyInjection
in the code sample? We do sth similar for Npgsql: https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/tree/master/src/HealthChecks.NpgSql#recommended-approach
And it would be nice to describe the breaking change in the README as well
services | ||
.AddHealthChecks() | ||
.AddNats( | ||
clientFactory: sp => |
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.
nit: some of our users may be searching for the samples in our test project. This code is valid, but it registers a factory that is going to create a new connection instance every time a health check is going to be invoked (which may lead to resource leaks etc).
Would you have some time to use NATS.Extensions.Microsoft.DependencyInjection instead and just call AddNatsClient
from that package?
Here I've found some samples (in a test project ;)): https://github.com/nats-io/nats.net/blob/3f919ddcd55ea0f765b4ce9b4137e2a58c589683/tests/NATS.Extensions.Microsoft.DependencyInjection.Tests/NatsHostingExtensionsTests.cs#L73
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 did not use AddNatsClient
in this test because I wanted to test the client factory.
But you are right. It may lead to resource leaks. So, I kept the client factory but changed it to return the created client, not to create a new one.
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, thank you very much for your contribution @Alirexaa !
What this PR does / why we need it:
This pull request introduces several significant changes to the NATS health check implementation. The updates include replacing the
NATS.Client
package withNATS.Net
, refactoring theNatsHealthCheck
class to use the new client, and updating the health check registration and tests accordingly.Dependency updates:
NATS.Client
package withNATS.Net
package in project files and updated the relevant using directives. [1] [2] [3] [4]Code refactoring:
NatsHealthCheck
class to useINatsConnection
fromNATS.Client.Core
and simplified the health check logic.NatsOptions
class, as it is no longer needed with the newNATS.Net
client.Health check registration:
NatsHealthCheckBuilderExtensions
to support a client factory for creatingINatsConnection
instances and simplified the method signatures.Tests:
NATS.Net
client and modified the test cases to reflect the changes in the health check registration and implementation. [1] [2]Which issue(s) this PR fixes:
Please reference the issue this PR will close: #[issue number]
Special notes for your reviewer:
It's recommended to use NATS.Net for .NET 6 upwards.
Also, we use the same approach (#2040) to register client singleton.
Also, this PR should fix #2199
Does this PR introduce a user-facing change?:
Yes.
NATS.Client
removed. All previewsAddNats
were removed and a new one using the factory method was introduced.Please make sure you've completed the relevant tasks for this PR, out of the following list: