-
Notifications
You must be signed in to change notification settings - Fork 70
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
Bump go-mysql dependency #187
Conversation
github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726 // indirect | ||
github.com/siddontang/go-log v0.0.0-20180807004314-8d05993dda07 | ||
github.com/siddontang/go-mysql v0.0.0-20180802024903-58848a70cf1a | ||
github.com/sirupsen/logrus v1.0.4 | ||
github.com/siddontang/go-mysql v0.0.0-20200424072754-803944a6e4ea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to audit the changes from the version we pin until this new version as this is a high risk change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, kind of understood that, 89 commits, 4,656 additions and 18,017 deletions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look through that list and pull out the highlights in a comment below.
go-mysql changes go-mysql-org/go-mysql@58848a7...803944a
Nothing looks too crazy. MySQL 8 changes is great, but pretty new. I haven't had a chance to review all the code in PR 468 and if it would have any impact on us as we don't use MySQL 8. Some bug fixes in Canal caught my eye as we independently did this. Also saw a lot of work on GTID stuff that we don't use. |
Also interesting to note that once we merge this theoretically we can support MySQL 8... but that requires extensive validation... |
Changes look good to me. I've been testing this branch using copydb on a dry-run attempt to move 1 TB of data via UNIX sockets. Everything went smooth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM if Shuhao is happy with the upstream changes
Current go-mysql vendor is from 2018 and don't include support for UNIX sockets. This was fixed 15 commits after our frozen commit. This pull will bring in the latest go-mysql.
We also set source and target port to 0 when using UNIX sockets since this is how go-mysql distinguish a UNIX config:
https://github.com/siddontang/go-mysql/blob/master/replication/binlogsyncer.go#L829