Add ClickHouse HealthCheck support #2315
Conversation
github-actions
bot
added
github_actions
smbecker
force-pushed
the
sb-clickhouse
branch
from
b7411b3 to
cb89eac
Compare
There was a problem hiding this comment.
@smbecker big thanks for your contribution! It looks like a step in the right direction, we just need to clarify how ClickHouseConnection should be used to ensure we provide the right API and of course solve the copy-paste errors.
| { | ||
| try | ||
| { | ||
| await using var connection = _options.ConnectionFactory?.Invoke() ?? new ClickHouseConnection(_options.ConnectionString); |
There was a problem hiding this comment.
What are the recommendations for using ClickHouseConnection? Namely:
- is it thread safe?
- does it have a built-in pool of connections? (when we execute
new ClickHouseConnection(_options.ConnectionString), does it always create a new coonection?)
I need to get a better understanding of how it should be used before I approve the shape of the public API for this DB.
There was a problem hiding this comment.
is it thread safe?
Looking at the code, instance methods on ClickHouseConnection do not appear to be thread-safe. However, commands do appear to be thread-safe in that they just execute HttpRequestMessages against the owned HttpClient.
does it have a built-in pool of connections?
It does not support pooling directly. However, when creating a ClickHouseConnection with a passed in HttpClient or IHttpClientFactory, you can take advantage of pooling within the HttpClient.
I could follow a similar pattern to Npgsql if/when this PR is merged.
There was a problem hiding this comment.
@smbecker thank you for checking everything and contributing a PR!
Based on what you wrote and https://github.com/DarkWanderer/ClickHouse.Client/wiki/Connection-lifetime-&-pooling#recommendations:
If an application operates on large volumes of transactions and requires to create/destroy ClickHouseConnection objects often, it is recommended to use IHttpClientFactory or a static instance of HttpClient to manage connections
I wonder if we should do the following:
- Recommend the users to use
IHttpClientFactoryin the README.md and show an example that registersClickHouseConnectionfactory method along with theIHttpClientFactory(people just copy this code most of the time, this is why I would like it to be the "right" way of doing things) - Have a single extension method that takes a mandatory
Func<IServiceProvider, ClickHouseConnection> connectionFactoryargument. It allows the user to obtain all kinds of configuration options from the service provider and forces them to createClickHouseConnectionon their own.
public static IHealthChecksBuilder AddClickHouse(
this IHealthChecksBuilder builder,
Func<IServiceProvider, ClickHouseConnection> connectionFactory,
string healthQuery = HEALTH_QUERY,
Action<ClickHouseConnection>? configure = null,
string? name = default,
HealthStatus? failureStatus = default,
IEnumerable<string>? tags = default,
TimeSpan? timeout = default)@smbecker what do you think?
There was a problem hiding this comment.
PTAL at my comments. Overall the PR is high quality contribution, I just want us to make sure we introduce the best API design from day one.
Again, thank you @smbecker !
| { | ||
| try | ||
| { | ||
| await using var connection = _options.ConnectionFactory?.Invoke() ?? new ClickHouseConnection(_options.ConnectionString); |
There was a problem hiding this comment.
@smbecker thank you for checking everything and contributing a PR!
Based on what you wrote and https://github.com/DarkWanderer/ClickHouse.Client/wiki/Connection-lifetime-&-pooling#recommendations:
If an application operates on large volumes of transactions and requires to create/destroy ClickHouseConnection objects often, it is recommended to use IHttpClientFactory or a static instance of HttpClient to manage connections
I wonder if we should do the following:
- Recommend the users to use
IHttpClientFactoryin the README.md and show an example that registersClickHouseConnectionfactory method along with theIHttpClientFactory(people just copy this code most of the time, this is why I would like it to be the "right" way of doing things) - Have a single extension method that takes a mandatory
Func<IServiceProvider, ClickHouseConnection> connectionFactoryargument. It allows the user to obtain all kinds of configuration options from the service provider and forces them to createClickHouseConnectionon their own.
public static IHealthChecksBuilder AddClickHouse(
this IHealthChecksBuilder builder,
Func<IServiceProvider, ClickHouseConnection> connectionFactory,
string healthQuery = HEALTH_QUERY,
Action<ClickHouseConnection>? configure = null,
string? name = default,
HealthStatus? failureStatus = default,
IEnumerable<string>? tags = default,
TimeSpan? timeout = default)@smbecker what do you think?
smbecker
force-pushed
the
sb-clickhouse
branch
from
07c7f07 to
55eda45
Compare
|
I took your recommendations for the most part. Please take a look and let me know if I missed anything. |
There was a problem hiding this comment.
LGTM, thank you very much for your contribution @smbecker !
|
Thanks for collaborating on it with me! |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2315 +/- ##
==========================================
- Coverage 66.88% 66.65% -0.24%
==========================================
Files 268 255 -13
Lines 8730 8600 -130
Branches 631 617 -14
==========================================
- Hits 5839 5732 -107
+ Misses 2723 2702 -21
+ Partials 168 166 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
What this PR does / why we need it: Adds health check support for ClickHouse
Does this PR introduce a user-facing change?: It adds a new package to support ClickHouse health checks
Please make sure you've completed the relevant tasks for this PR, out of the following list: