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

draft: update clickhouse-go to v2 #562

Closed
wants to merge 2 commits into from
Closed

draft: update clickhouse-go to v2 #562

wants to merge 2 commits into from

Conversation

anikin-aa
Copy link
Contributor

@anikin-aa anikin-aa commented Nov 10, 2022

Update clickhouse-go module to v2. Fix integration tests according to new contract from the lib

@anikin-aa
Copy link
Contributor Author

local build is ok, unit tests are also OK. will try launch integration tests today and share the results

@anikin-aa anikin-aa changed the title update clickhouse-go to v2 draft update clickhouse-go to v2 Nov 10, 2022
@anikin-aa anikin-aa changed the title draft update clickhouse-go to v2 draft: update clickhouse-go to v2 Nov 10, 2022
@Slach
Copy link
Collaborator

Slach commented Nov 16, 2022

Integration tests failed
look details
here
https://github.com/AlexAkulov/clickhouse-backup/actions/runs/3434851457/jobs/5814860566#step:7:2539
and here
https://github.com/AlexAkulov/clickhouse-backup/actions/runs/3434851457/jobs/5814860928#step:7:110

Could you change /test/integration/integration_test.go for use clickhouse-go/v2?

Copy link
Collaborator

@Slach Slach left a comment

Choose a reason for hiding this comment

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

could we return error in chbackend.Query after switch to v2?

@anikin-aa
Copy link
Contributor Author

Could you change /test/integration/integration_test.go for use clickhouse-go/v2?

yep, i've missed this file, working on it

could we return error in chbackend.Query after switch to v2?

https://github.com/anikin-aa/clickhouse-backup/blob/clickhouse-go/v2/pkg/clickhouse/clickhouse.go#L724-L726 error already returned, need to do changes in /test/integration/integration_test.go

@Slach
Copy link
Collaborator

Slach commented Nov 22, 2022

@anikin-aa any progress?

@anikin-aa
Copy link
Contributor Author

anikin-aa commented Nov 23, 2022

@Slach , hi !
new changes has been committed, local tests are passed

PASS
ok  	command-line-arguments	214.196s

there is one ugly part here: https://github.com/anikin-aa/clickhouse-backup/blob/clickhouse-go/v2/test/integration/integration_test.go#L1713-L1726

my be you could suggest better approach ?

@Slach
Copy link
Collaborator

Slach commented Nov 23, 2022

;((
looks like clickhouse-go v2 doesn't work with old clickhouse-server version ;(
https://github.com/AlexAkulov/clickhouse-backup/actions/runs/3529409950/jobs/5925048227#step:7:120

Don't sure we should to approve this PR in this case
which improvements it will add to main use cases?

Maybe we should switch from native to http protocol?

@anikin-aa
Copy link
Contributor Author

which improvements it will add to main use cases?

Not so much to be honest, just take the latest available library

Maybe we should switch from native to http protocol?

http right now set as experimental for clickhouse-go https://github.com/ClickHouse/clickhouse-go#https-experimental

I will try to check what's possible to do with old ch versions

@Slach
Copy link
Collaborator

Slach commented Apr 19, 2023

@anikin-aa
maybe after merge ClickHouse/clickhouse-go#975
next release clickhouse-go should works with old clickhouse versions

@Slach
Copy link
Collaborator

Slach commented Apr 19, 2023

lets wait and resolve conflicts

@Slach
Copy link
Collaborator

Slach commented Jun 11, 2023

lookg progress #669

@Slach Slach closed this Jun 11, 2023
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.

None yet

2 participants