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.

Sign up for GitHub

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

Jump to bottom

Upgrade to clickhouse-go/v2 #5020

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Upgrade to clickhouse-go/v2 #5020

merged 1 commit into from
Jun 6, 2023

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented May 22, 2023 •
edited
Loading

We upgrade Flow Aggregator to use ClickHouse go client v2 to better assist future supports. In terms of the method of creating connection, we replace DSN with OpenDB in database/sql interface.

Fixes: #4901

@heanlan heanlan marked this pull request as draft May 23, 2023 17:19
@heanlan
Copy link
Contributor Author

heanlan commented May 23, 2023 •
edited
Loading

Put this PR on hold as we will have to upgrade clickhouse-server version first. The current clickhouse-server is v22.6 in Theia, which is incompatible with the upgraded clickhouse-client. I'm fixing some issues in Theia while upgrading clickhouse-server.

@heanlan heanlan marked this pull request as ready for review June 1, 2023 17:15
@heanlan
Copy link
Contributor Author

heanlan commented Jun 1, 2023

Put this PR on hold as we will have to upgrade clickhouse-server version first. The current clickhouse-server is v22.6 in Theia, which is incompatible with the upgraded clickhouse-client. I'm fixing some issues in Theia while upgrading clickhouse-server.

We decided to remove the dependency from Antrea to Theia - by using official clickhouse-server images in e2e tests, instead of using theia/clickhouse-server images, which includes data schema management plugin.

dreamtalen
dreamtalen previously approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

LGTM

yuntanghsu
yuntanghsu previously approved these changes Jun 1, 2023
View reviewed changes
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
yanjunz97 reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only a nit

build/yamls/flow-visibility-e2e.yml Show resolved Hide resolved
antoninbas
antoninbas reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I amy

build/charts/flow-aggregator/values.yaml Outdated Show resolved Hide resolved
pkg/flowaggregator/clickhouseclient/clickhouseclient.go Show resolved Hide resolved
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jun 1, 2023
@heanlan heanlan dismissed stale reviews from yuntanghsu and dreamtalen via afa5323 June 2, 2023 02:48
yanjunz97
yanjunz97 previously approved these changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

LGTM

@heanlan heanlan requested a review from antoninbas June 2, 2023 18:31
yuntanghsu
yuntanghsu previously approved these changes Jun 2, 2023
View reviewed changes
@heanlan heanlan dismissed stale reviews from yuntanghsu and yanjunz97 via 220b323 June 2, 2023 20:06
antoninbas
antoninbas previously approved these changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/flowaggregator/exporter/clickhouse_test.go Outdated Show resolved Hide resolved
@heanlan
Upgrade to clickhouse-go/v2
4bce8b8 
We upgrade Flow Aggregator to use ClickHouse go client v2 to
better assist future supports. In terms of the method of creating
connection, we replace DSN with OpenDB in database/sql interface.

Fixes: antrea-io#4901
Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Copy link
Contributor Author

heanlan commented Jun 3, 2023

/test-all

@heanlan
Copy link
Contributor Author

heanlan commented Jun 5, 2023

/test-e2e

@heanlan heanlan requested a review from antoninbas June 5, 2023 21:22
@heanlan
Copy link
Contributor Author

heanlan commented Jun 5, 2023

I realized the jenkins test is not testing on FlowAggregator overall, no need to trigger it. Although I don't remember when we added --flow-visibility flag into kind test in PR #3673 , why we didn't do the same for jenkins, worth revisit later.

http://10.176.27.169:8080/job/antrea-e2e-for-pull-request/2107/consoleFull

=== RUN   TestFlowAggregator
    fixtures.go:54: Skipping when not running flow visibility test
--- SKIP: TestFlowAggregator (0.00s)

@antoninbas
Copy link
Contributor

jenkins-e2e is broken apparently, so I'll go ahead and merge this

@antoninbas antoninbas merged commit 3d1eb00 into antrea-io:main Jun 6, 2023
@heanlan heanlan deleted the ch-client-upgrade branch June 6, 2023 00:30
@heanlan heanlan mentioned this pull request Jun 6, 2023
Closed
@antoninbas antoninbas mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanjunz97 yanjunz97 yanjunz97 left review comments

@yuntanghsu yuntanghsu yuntanghsu left review comments

@antoninbas antoninbas antoninbas approved these changes

@dreamtalen dreamtalen Awaiting requested review from dreamtalen

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FlowAggregator] Upgrade to ClickHouse Go client v2
5 participants
@heanlan @antoninbas @dreamtalen @yanjunz97 @yuntanghsu