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

Add CassandraDb HealthCheck support #2174

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

djesusnet
Copy link

What this PR does / why we need it: Adds health check support for CassandraDb

Does this PR introduce a user-facing change?: It adds a new package to support CassandraDb health checks

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@github-actions github-actions bot added github_actions Pull requests that update Github_actions code test labels Mar 9, 2024
@djesusnet djesusnet marked this pull request as draft March 9, 2024 11:59
@djesusnet djesusnet changed the title Add CassandraDb HealthCheck support Add CassandraDb HealthCheck support Mar 9, 2024
@djesusnet djesusnet marked this pull request as ready for review March 9, 2024 12:17
@djesusnet djesusnet closed this Mar 9, 2024
@djesusnet djesusnet reopened this Mar 9, 2024
@djesusnet
Copy link
Author

@dotnet-policy-service agree

@djesusnet
Copy link
Author

@esskar @manne @Daniel15 @inkel , Could you please review this PR when you have some time?

@inkel
Copy link
Contributor

inkel commented Oct 23, 2024

I'd love to! But I haven't written any .NET code in years, and I'm not a maintainer of this project, sorry. FWIW the code LGTM.

@djesusnet
Copy link
Author

@sungam3r Could you review my PR?

Copy link
Collaborator

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Overall it's welcomed and high quality contribution. Thank you @djesusnet !

We need to agree on the shape of the API. Please see my comment and let me know what you think about it.

{
try
{
var builder = Cluster.Builder().AddContactPoint(_options.ContactPoint);
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the current approach, we would build a new instance every time the health check would be invoked. Which I expect would be bad for performance and could even lead to some resource leaks (nothing is getting disposed here).

We have changed many of our health checks similar to what I described in #2040: for all the client instances that are thread-safe and should be used as singleton, we don't create our own instances, but expect it to be resolved from the DI (so the user registers a singleton and we just resolve).

I've done some quick web search, but have failed to find dependency injection best practices for working with the C# client of CassandarDb.

Should we just expect the users to inject the cluster into the DI and solve it?

For example this is what we do for MongoDB:

We allow the users to specify a Func<IServiceProvider, IMongoClient> factory:

when it's not specified, we just try to resolve it from the DI container:

IMongoClient client = clientFactory?.Invoke(sp) ?? sp.GetService<MongoClient>() ?? sp.GetRequiredService<IMongoClient>();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants