Skip to content

Upgrade go minimum to 1.15#6792

Closed
guidoiaquinti wants to merge 5 commits intovitessio:masterfrom
guidoiaquinti:master
Closed

Upgrade go minimum to 1.15#6792
guidoiaquinti wants to merge 5 commits intovitessio:masterfrom
guidoiaquinti:master

Conversation

@guidoiaquinti
Copy link
Copy Markdown
Member

Description

This bumps the minimum golang version to 1.15. For prior art, the PR to bump to 1.12 was #5017 and for 1.13 was #5943.

➜  vitess git:(master) grep -l -RF 1.13 .
./go/vt/vtctld/rice-box.go
./go/vt/orchestrator/external/raft/LICENSE
./web/vtctld2/app/plotly-latest.min.js
./web/vtctld2/package-lock.json
./web/vtctld2/src/plotly-latest.min.js
./go.sum
./.github/workflows/docker_test_2.yml
./doc/releasenotes/7_0_0_release_notes.md

Once this is approved and merged I'll open a similar PR for the vitessio/website repo like vitessio/website#419

Why

Go 1.14 release "improves the performance of most uses of defer to incur almost zero overhead compared to calling the deferred function directly. As a result, defer can now be used in performance-critical code without overhead concerns".

Go 1.15 release "includes substantial improvements to the linker, improves allocation for small objects at high core counts [...]".

Signed-off-by: Guido Iaquinti <giaquinti@slack-corp.com>
@guidoiaquinti
Copy link
Copy Markdown
Member Author

Not very familiar with the new CI but are those tests failing because they rely on Docker images (built on 1.13)?

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Sep 29, 2020

Not very familiar with the new CI but are those tests failing because they rely on Docker images (built on 1.13)?

Some of them yes, but some seem to be compile errors in test code:

Error: go/vt/vtgate/vcursor_impl_test.go:280:9: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)

You should be able to click through to Details on each of the CI checks. This one is from unit and unit_race.

@derekperkins
Copy link
Copy Markdown
Member

I forgot that we hadn't upgraded. I think there are some significant gains to be had with the new defer optimizations.

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Sep 29, 2020

@derekperkins @dkhenry what are the steps needed to get the tests passing? build bootstrap images off this PR and push them to dockerhub? That will break other open PRs until this PR is merged, correct?

@deepthi deepthi added this to the v8.0 milestone Sep 29, 2020
@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Sep 30, 2020

I have merged #6800. A merge/rebase should get unit tests passing on this PR.

@derekperkins
Copy link
Copy Markdown
Member

@deepthi I wouldn't expect this to break other open PRs unless they're hitting some strange edge cases.

Guido Iaquinti added 2 commits September 30, 2020 13:15
@guidoiaquinti
Copy link
Copy Markdown
Member Author

All the CI tests still failing are due to:

ERROR: Go version reported: go version go1.13.15 linux/amd64. Version 1.15+ required. See https://vitess.io/contributing/build-from-source for install instructions.

This is because we run the test using pre-built images (like vitess/bootstrap:mysql57) instead of rebuilding those against master before launching the suite.

@guidoiaquinti
Copy link
Copy Markdown
Member Author

Also, I missed that there’s no official go 1.15 docker image for Debian Stretch so for the upgrade we also need to move to Debian Buster. This is not super straight forward as there are major deprecations, package name changes and so on.

@morgo worked on the Debian upgrade in #6129 but it was never pushed to the finish line. I'll see if I can revamp his PR but till it's merged I think we can consider this PR blocked. Feedback/ideas are welcome.

@dkhenry
Copy link
Copy Markdown
Contributor

dkhenry commented Oct 15, 2020

Just doing some sanity tests before we do this merge

Go 1.14.3

TPCC - 2972.80

Go 1.15.3

TPCC - 2906.25

So we are in the same ballpark ( we know theses tests have been giving 20% variance between runs ). I am going to continue to test to see if I can get definitive numbers, but this should be clear to merge once the new bootstraps get pushed and the CI builds pass

@guidoiaquinti do you need me to push up new bootstrap images ?

@guidoiaquinti
Copy link
Copy Markdown
Member Author

Just doing some sanity tests before we do this merge

Go 1.14.3

TPCC - 2972.80

Go 1.15.3

TPCC - 2906.25

So we are in the same ballpark ( we know theses tests have been giving 20% variance between runs ). I am going to continue to test to see if I can get definitive numbers, but this should be clear to merge once the new bootstraps get pushed and the CI builds pass

@guidoiaquinti do you need me to push up new bootstrap images ?

Hey @dkhenry, thanks for your comment. Were you doing your tests against this branch? Because I think we need #6833 before merging this as there are no go 1.15 docker image for Debian Stretch.

Let me know if I've missed anything. Thanks!

@deepthi deepthi modified the milestones: v8.0, v9.0 Nov 2, 2020
@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Jan 12, 2021

replaced with #7204

@deepthi deepthi closed this Jan 12, 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.

4 participants