-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support ClickHouse Cluster deployment #55
Conversation
6fe4703
to
b42d594
Compare
Do we need to add podDistribution for clickhouse pod? Like ClickHouseAntiAffinity/ShardAntiAffinity/ReplicaAntiAffinity? |
Thanks Yun-Tang for the suggestion! Add a default podDistribution of type |
docs/network-flow-visibility.md
Outdated
3 replicas will be deployed for ClickHouse cluster. Please make sure there are | ||
at least 3 nodes available in your Kubernetes cluster. To use a customized | ||
ZooKeeper cluster, please refer the | ||
[ClickHouse instructions](https://github.com/Altinity/clickhouse-operator/blob/master/docs/zookeeper_setup.md) |
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.
I feel the name of the link ClickHouse instructions
can be more descriptive/specific, same for L245
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.
Make sense! I update it to ZooKeeper setup instructions for ClickHouse
. Please tell me if you have other suggestion, thanks!
3bac901
to
a526502
Compare
LGTM |
5bb74fa
to
74d037c
Compare
build/charts/theia/templates/clickhouse/zookeeper/statefulset.yaml
Outdated
Show resolved
Hide resolved
build/charts/theia/templates/clickhouse/nfs-persistentvolume.yaml
Outdated
Show resolved
Hide resolved
31d50b1
to
851b558
Compare
851b558
to
427b1d7
Compare
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.
Don't have much to add in terms of comments, as the change LGTM.
We need to finalize on our strategy regarding support for non-clustered Clickhouse and then we can finalize this PR.
build/charts/theia/README.md
Outdated
@@ -18,6 +18,14 @@ Kubernetes: `>= 1.16.0-0` | |||
|
|||
| Key | Type | Default | Description | | |||
|-----|------|---------|-------------| | |||
| clickhouse.cluster.enable | bool | `false` | Determine whether to deploy ClickHouse as a cluster. | |
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.
While we discuss on whether we consider non-clustered supported, do you think we can enable clustering by 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.
+1
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.
As we plan to deprecate the non-clustered, I think it might be good to remove cluster.enable
here and always deploy ClickHouse cluster. Updated as this way.
build/charts/theia/README.md
Outdated
| clickhouse.storage.createPersistentVolume.local.path | string | `"/data"` | The local path. Required when type is "Local". | | ||
| clickhouse.storage.createPersistentVolume.nfs.host | string | `""` | The NFS server hostname or IP address. Required when type is "NFS". | | ||
| clickhouse.storage.createPersistentVolume.nfs.path | string | `""` | The path exported on the NFS server. Required when type is "NFS". | | ||
| clickhouse.storage.createPersistentVolume.nfs.hosts | list | `[]` | A list of NFS server hostname or IP address. Required when type is "NFS". When ClickHouse clustering is enabled, please provide (shards * replicas) NFS servers. Otherwise, only one NFS server is required. | |
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.
I think we're not providing NFS servers but NFS shares, is that correct? They could be for instance all on the same server, mapped to locations assigned to different physical drives.
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.
An NFS share could be defined as nfs.hosts:nfs.path
. Do you think this is better to let user input a list of nfs.hosts:nfs.path
instead of accepting the two values separately, so that user can use either multiple servers or single server with multiple drives?
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.
Updated as above, thanks
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
=======================================
Coverage ? 24.72%
=======================================
Files ? 9
Lines ? 1278
Branches ? 0
=======================================
Hits ? 316
Misses ? 940
Partials ? 22
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
e35793c
to
f5fe273
Compare
build/charts/theia/README.md
Outdated
@@ -18,6 +18,14 @@ Kubernetes: `>= 1.16.0-0` | |||
|
|||
| Key | Type | Default | Description | | |||
|-----|------|---------|-------------| | |||
| clickhouse.cluster.enable | bool | `false` | Determine whether to deploy ClickHouse as a cluster. | |
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.
+1
docs/network-flow-visibility.md
Outdated
To deploy ClickHouse as a cluster, please set `clickhouse.cluster.enable` to true | ||
and set `clickhouse.cluster.shards` and `clickhouse.cluster.replicas` per your | ||
requirement. Please notice that a standlone ClickHouse server cannot switch to | ||
ClickHouse cluster without deleting. If you plan to scale the ClickHouse cluster |
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.
Should we mention that it also will lose data if user wants to switch a cluster to a standalone one?
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.
It makes sense. Added, thanks!
docs/network-flow-visibility.md
Outdated
can deploy multiple replicas in a shard to ensure reliability. | ||
|
||
To deploy ClickHouse as a cluster, please set `clickhouse.cluster.enable` to true | ||
and set `clickhouse.cluster.shards` and `clickhouse.cluster.replicas` per your |
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 just mainly explained what was shard, but replica is a new term now. Not sure if it is easy for users to understand the relationships between shard and replica.
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.
Added some explanations for replicas. Could you check if it is clearer now? Thank!
14d8a55
to
7c207a6
Compare
50b3428
to
6ffcd0f
Compare
/theia-test-e2e |
6ffcd0f
to
27899ff
Compare
/theia-test-e2e |
27899ff
to
1ab3c23
Compare
/theia-test-e2e |
1ab3c23
to
bb71e47
Compare
@salv-orlando @ziyouw I have deprecated the non-cluster mode in this PR and verify the e2e tests. Would you like to take another look at this PR? |
254c061
to
769ee74
Compare
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.
Awesome work, I just have some comments about documentation
7ae45b7
to
323529c
Compare
/theia-test-e2e |
6431c20
to
0f093f6
Compare
0f093f6
to
eb92fc5
Compare
Signed-off-by: Yanjun Zhou <zhouya@vmware.com>
eb92fc5
to
aab4cc6
Compare
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.
LGTM
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.
LGTM
Fix #53
This PR adds support for ClickHouse cluster deployment with shards and replicas.
Signed-off-by: Yanjun Zhou zhouya@vmware.com