[exporter/clickhouse] change how default database is read from config #33693
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
This was referenced Jun 21, 2024
dmitryax
approved these changes
Jun 21, 2024
TylerHelmuth
pushed a commit
that referenced
this pull request
Jul 1, 2024
…onfig option. (#33614) ### DEPENDS ON #33693, #33694 **Description:** Sets `async_insert` to true by default to enable [asynchronous inserts](https://clickhouse.com/docs/en/optimize/asynchronous-inserts). Because this value is being given a default, I have added a config option under the same name. Keep in mind that if `async_insert` is provided in `endpoint` or `connection_params` it will take precedence and ignore this new config option. This is similar to how the `database` config option behaves. The goal is to provide better insert performance by default, since not all users will know to set it in their DSN URL. This also opens the discussion to ___**whether or not this is a breaking change**___. Depending on the deployment's telemetry throughput, this could be an unexpected change that leads to [`TOO_MANY_PARTS`](https://clickhouse.com/docs/knowledgebase/exception-too-many-parts) errors. I don't expect this to be the case however, but I welcome any discussion about this concern. This PR is being resubmitted with suggestions from @crobert-1 and @dmitryax applied. Here are the extra changes with these suggestions applied: - Extracted unrelated changes into separate PRs - Updated `async_insert` to avoid using a `bool` pointer - Updated tests to be able to support these non-pointer-yet-still-optional test cases **Testing:** Ran integration tests. Also added an abundance of tests to check the behavior of `async_insert` when present in `endpoint`, `connection_params`, and exporter config. **Documentation:** - Updated README for all related changes Unrelated change, also updated README's SQL samples to use `sql` instead of `clickhouse` for the code samples to enable proper syntax highlighting. ClickHouse SQL is compatible with plain SQL.
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.
EXTRACTED FROM #33614
Description:
Changes how the database is read from the config. The intent is to simplify the behavior, and give a proper default value. In the current version it's hard to distinguish in tests between empty string vs default.
Testing:
Updated tests, and test structure for parse DSN