-
Notifications
You must be signed in to change notification settings - Fork 544
Dapr state store clickhouse #3675
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mehmet TOSUN <[email protected]>
|
Thanks @middt, implementation generally looks good to me! Would it be possible to add conformance tests for this too? It seems like we should be able to spin up a click house docker compose. |
…tation Signed-off-by: Mehmet TOSUN <[email protected]>
…tion handling Signed-off-by: Mehmet TOSUN <[email protected]>
… connection handling Signed-off-by: Mehmet TOSUN <[email protected]>
1b2a8de to
2c8934c
Compare
|
Thank you for the feedback! @JoshVanL I've implemented the conformance tests for the ClickHouse state store in this PR:
The tests verify all the key functionality including:
I've also addressed authentication issues by properly configuring username and password in both the Docker Compose setup and the state store implementation. All unit tests are now passing, confirming that the implementation works correctly with the ClickHouse server. |
Signed-off-by: Mehmet TOSUN <[email protected]>
Signed-off-by: Mehmet TOSUN <[email protected]>
|
Thanks @middt, I think the only thing left is to do a Appreciate the work on this! |
|
Hey @middt, tests are failing |
- Add environment variable check for RUN_CLICKHOUSE_INTEGRATION_TEST - Skip integration tests when ClickHouse is not available - This fixes CI test failures when ClickHouse server is not running Signed-off-by: Mehmet TOSUN <[email protected]>
Signed-off-by: Mehmet TOSUN <[email protected]>
755a9f2 to
9526231
Compare
…tests Signed-off-by: Mehmet TOSUN <[email protected]>
4f3e99d to
e35c604
Compare
Signed-off-by: Mehmet TOSUN <[email protected]>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
sicoyle
left a comment
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.
few comments. Nice job 👏
| @@ -0,0 +1,435 @@ | |||
| /* | |||
| Copyright 2021 The Dapr Authors | |||
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.
| Copyright 2021 The Dapr Authors | |
| Copyright 2025 The Dapr Authors |
| // Create database if not exists | ||
| // Note: Database and table names are validated during metadata parsing | ||
| // and come from trusted configuration, so direct string concatenation is acceptable here | ||
| createDBQuery := "CREATE DATABASE IF NOT EXISTS " + c.config.DatabaseName |
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.
we need to make sure this behavior is documented. Have you created a docs PR for these changes by chance? We try to have a docs PR open before merging new components please 🙏
| if config.DatabaseName == "" { | ||
| return config, errors.New("ClickHouse database name is missing") | ||
| } | ||
|
|
||
| if config.TableName == "" { |
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.
are there fixed char limits on these we should check for by chance?
| @@ -0,0 +1,569 @@ | |||
| /* | |||
| Copyright 2021 The Dapr Authors | |||
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.
| Copyright 2021 The Dapr Authors | |
| Copyright 2025 The Dapr Authors |
| @@ -0,0 +1,240 @@ | |||
| /* | |||
| Copyright 2021 The Dapr Authors | |||
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.
| Copyright 2021 The Dapr Authors | |
| Copyright 2025 The Dapr Authors |
| - title: "Username and password" | ||
| description: "Authenticate using username and password." | ||
| metadata: | ||
| - name: Username | ||
| type: string | ||
| required: false | ||
| description: | | ||
| Username for ClickHouse host. Defaults to empty. | ||
| example: "default" | ||
| default: "" | ||
| - name: Password | ||
| type: string | ||
| required: false | ||
| sensitive: true | ||
| description: | | ||
| Password for ClickHouse host. No default. Use secretKeyRef for | ||
| secret reference | ||
| example: "mypassword" | ||
| default: "" |
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.
this says that none of the fields for auth are required. Can you please confirm if this is this true that we can connect without a username and password?
| example: "mypassword" | ||
| default: "" | ||
| metadata: | ||
| - name: ClickhouseURL |
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.
can you pls update these all to be camelcase?
| // +build conftests | ||
|
|
||
| /* | ||
| Copyright 2021 The Dapr Authors |
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.
| Copyright 2021 The Dapr Authors | |
| Copyright 2025 The Dapr Authors |
|
@JoshVanL did you have any other feedback here by chance? |
Description
This PR adds a new state store component for ClickHouse, a column-oriented database management system. The ClickHouse state store component provides the following features:
Key implementation details:
Implementation Details
The component includes:
State store implementation (
clickhouse.go)Tests (
clickhouse_test.go)Configuration options:
Checklist
Testing Done
Tests were run against ClickHouse v23.8 using the official Go driver.
Additional Notes