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

Adopt Golang version ClickHouse data schema management #95

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

yanjunz97
Copy link
Contributor

@yanjunz97 yanjunz97 commented Aug 24, 2022

We introduce the ClickHouse data schema management code implemented
in shell script in v0.2.0. In terms of the future maintenance, this PR
re-implements this function in Golang with the library golang-migrate.

Signed-off-by: Yanjun Zhou zhouya@vmware.com

@yanjunz97 yanjunz97 force-pushed the clickhouse-go-migration branch 2 times, most recently from eecf16a to 4900074 Compare August 24, 2022 19:40
@yanjunz97
Copy link
Contributor Author

/theia-test-e2e

@yanjunz97 yanjunz97 marked this pull request as ready for review August 24, 2022 21:20
@yanjunz97 yanjunz97 added this to the Theia v0.3 Release milestone Aug 25, 2022
@yanjunz97 yanjunz97 force-pushed the clickhouse-go-migration branch 2 times, most recently from ad9994c to 79d8d49 Compare August 30, 2022 21:49
ci/kind/test-e2e-kind.sh Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
@yanjunz97 yanjunz97 force-pushed the clickhouse-go-migration branch 2 times, most recently from ca766e6 to f38cd5a Compare September 1, 2022 00:23
@yanjunz97
Copy link
Contributor Author

/theia-test-e2e

@yanjunz97 yanjunz97 force-pushed the clickhouse-go-migration branch 3 times, most recently from 20d716a to afadc34 Compare September 3, 2022 01:27
@yanjunz97
Copy link
Contributor Author

/theia-test-e2e

1 similar comment
@yanjunz97
Copy link
Contributor Author

/theia-test-e2e

@yanjunz97 yanjunz97 force-pushed the clickhouse-go-migration branch 3 times, most recently from 0244cfd to 17f0c1b Compare September 15, 2022 17:47
@codecov-commenter
Copy link

Codecov Report

Merging #95 (17f0c1b) into main (c9ef257) will increase coverage by 5.47%.
The diff coverage is 68.87%.

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
+ Coverage   30.65%   36.12%   +5.47%     
==========================================
  Files          11       12       +1     
  Lines        1442     1683     +241     
==========================================
+ Hits          442      608     +166     
- Misses        978     1034      +56     
- Partials       22       41      +19     
Flag Coverage Δ
unit-tests 36.12% <68.87%> (+5.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
plugins/clickhouse-schema-management/main.go 68.87% <68.87%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@yuntanghsu yuntanghsu left a comment

Choose a reason for hiding this comment

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

LGTM

@yanjunz97
Copy link
Contributor Author

As one of the reason we bump ClickHouse to v22.6 is that it supports both amd64 and arm64, I add a commit to keep this feature when we build our ClickHouse server image.

plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Outdated Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Show resolved Hide resolved
SETTINGS merge_with_ttl_timeout = {{ $ttlTimeout }};

--Move data from old table and drop old tables
INSERT INTO flows_local SELECT * FROM flows;
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, other modules also need to use flows_local instead of flows? Not sure if we need a way to keep them synced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most cases, the answer is no, as flows will be a distributed table have references of flows_local on each replica. Most modules will be able to use flows as the interface to read/write data. This change is introduced when we introduce the non-cluster mode, and we have already update the code in other modules that are influenced by this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also not new code, it's the migration code from 0.1 to 0.2. It shows up in this diff because it has been moved into a different place.

Copy link
Contributor

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

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

LGTM!

plugins/clickhouse-schema-management/main.go Show resolved Hide resolved
plugins/clickhouse-schema-management/main.go Show resolved Hide resolved
Signed-off-by: Yanjun Zhou <zhouya@vmware.com>
@yanjunz97 yanjunz97 merged commit 310a62d into antrea-io:main Sep 30, 2022
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.

5 participants