Use one Oximeter client per admin server #7171
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
Keeps the admin server's connections to ClickHouse from growing without bound. Also: don't automatically initialize an already-initialized ClickHouse database. This prevents data loss when upgrading. Schema updates for populated databases must be performed manually, as they have been in the past.
jgallagher
reviewed
Nov 26, 2024
clickhouse-admin/src/context.rs
Outdated
| let log = clickhouse_cli | ||
| .log | ||
| .as_ref() | ||
| .expect("should have configured logging via CLI or config"); |
There was a problem hiding this comment.
This seems fishy to me; can we make clickhouse_cli.log not optional so we don't need this unwrap? Or alternatively, could we accept a Logger that we pass through to the client instead of relying on getting it from clickhouse_cli?
There was a problem hiding this comment.
It seemed fishy because it was; thanks. 922157b makes ClickhouseCli.log non-optional.
| "can't read ClickHouse version: {e}", | ||
| )) | ||
| })?; | ||
| if version == 0 { |
There was a problem hiding this comment.
With apologies for this question possibly being more relevant to #6903 - how does this interact with Oximeter doing almost exactly (or maybe exactly exactly) this same check-if-0-and-initialize on startup, given the notes about initialization not being concurrency-safe?
There was a problem hiding this comment.
We'll take this as a follow-up: #7173.
karencfv
reviewed
Nov 26, 2024
There was a problem hiding this comment.
I recently came across documentation for ClickHouse's "startup scripts" https://clickhouse.com/docs/en/operations/startup-scripts. Essentially, this would mean we can define the schema directly on the XML configuration files.
<clickhouse>
<startup_scripts>
<scripts>
<query>CREATE DATABASE IF NOT EXISTS oximeter;</query>
</scripts>
<!-- More queries -->
</startup_scripts>
</clickhouse>Might be worth it to investigate if this works for us? If it does, we wouldn't have to initialize all the tables via the oximeter client.
plotnick
force-pushed
the
fix-clickhouse-admin-single
branch
from
1c282f9 to
922157b
Compare
|
Some positive results on |
jgallagher
approved these changes
Nov 27, 2024
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.
Keeps the admin server's connections to ClickHouse from growing without bound.
Also: don't automatically initialize an already-initialized ClickHouse database. This prevents data loss when upgrading. Schema updates for populated databases must be performed manually, as they have been in the past.