Skip to content
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

Adding support clickhouse cluster #568

Merged

Conversation

preved911
Copy link
Contributor

Hello. I would like to create cluster wide schema_migrations table, when I run migrations on clusterized clickhouse. As I think, It's will guarante that new migrations may be executed on any node of cluster.

@preved911 preved911 force-pushed the add_support_clickhouse_cluster_mode branch from 05f32e8 to 24cd4c7 Compare May 19, 2021 14:14
@coveralls
Copy link

coveralls commented May 19, 2021

Pull Request Test Coverage Report for Build 1159872900

  • 10 of 16 (62.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 57.425%

Changes Missing Coverage Covered Lines Changed/Added Lines %
database/clickhouse/clickhouse.go 10 16 62.5%
Totals Coverage Status
Change from base Build 1153229133: -0.03%
Covered Lines: 3693
Relevant Lines: 6431

💛 - Coveralls

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my future reference: clickhouse distributed ddl docs

Overall, this change LGTM. Keep in mind that you may have issues if migrations are run simultaneously on different machines since the clickhouse db driver doesn't use a distributed lock.

Can you add tests or is it not possible to setup a cluster in docker?

@preved911 preved911 force-pushed the add_support_clickhouse_cluster_mode branch 3 times, most recently from f9df314 to 4e2ae5b Compare May 23, 2021 00:10
@preved911
Copy link
Contributor Author

Can you add tests or is it not possible to setup a cluster in docker?

@dhui, for me its look like too hard. I viewed realized tests in project and can't imagine how to do that in such flow. For replicate tables over clickhouse nodes, we have to up 2 or more clickhouse nodes with customized config file(need zookeeper instance address specify). dktest, if I undestand correct, do not allow to mount volumes into declared containers. So, should I create very custom test for this case?

@dhui
Copy link
Member

dhui commented May 28, 2021

Yeah, I don't think it's a trivial amount of work to run multiple containers & images on the same network so we can test a clickhouse cluster setup.

I'd love to have integration tests for clickhouse clusters, but if it's not feasible, we can add a caveat in the docs stating that clickhouse clusters aren't officially supported since there aren't any tests.

@preved911 preved911 force-pushed the add_support_clickhouse_cluster_mode branch from 4e2ae5b to d312b4c Compare May 28, 2021 22:26
@preved911
Copy link
Contributor Author

@dhui, I added notes in README.md. Did you mean it?

@preved911
Copy link
Contributor Author

@dhui, so can it be merged?

@preved911 preved911 requested a review from dhui July 5, 2021 21:41
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay and thanks again for the PR!

@dhui dhui merged commit aa11941 into golang-migrate:master Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants