Skip to content

Support MySQL 5.7#1712

Merged
enisoc merged 7 commits into
vitessio:masterfrom
enisoc:mysql57
May 21, 2016
Merged

Support MySQL 5.7#1712
enisoc merged 7 commits into
vitessio:masterfrom
enisoc:mysql57

Conversation

@enisoc
Copy link
Copy Markdown
Contributor

@enisoc enisoc commented May 19, 2016

@alainjobart

This is mostly working now. There's only one test that I still need to fix (tabletmanager.py). There's also some warnings about performance_schema not getting bootstrapped, which I need to investigate.


This change is Reviewable

@alainjobart
Copy link
Copy Markdown
Contributor

alainjobart commented May 19, 2016

LGTM great stuff, thanks for investigating all the differences!

Approved with PullApprove

Anthony Yeh added 7 commits May 20, 2016 17:59
These options were removed entirely in MySQL 5.7.
The latter is deprecated. Also check for both pid and sock files to
disappear when shutting down, or else there is a race.
This causes mysqld to ignore relay logs on disk at startup,
and instead start replicating from the master wherever the SQL thread
left off.

This is generally recommended for safer crash recovery.
We also do it to avoid problems caused by spurious Previous-GTIDs lists
in relay logs that occur when we restart mysqld in integration tests.
@enisoc
Copy link
Copy Markdown
Contributor Author

enisoc commented May 21, 2016

The problem in tabletmanager.py was that when we restarted mysqld as part of the test, it remembered the wrong "Previous-GTIDs" value in the relay log, even though we had done RESET MASTER and RESET SLAVE before shutting down gracefully. Then when we restarted mysqld it would populate Retrieved_Gtid_Set with the wrong value, and replication would fail because the slave remembers phantom GTIDs that the master knows nothing about.

I wasn't able to find a way to force it to stop remembering the incorrect Previous-GTIDs value. I tried various combinations of rotating/flushing relay logs, etc. In the end, I decided to fix this by enabling relay_log_recovery, which tells MySQL to ignore previous relay logs on disk when it starts up. This is generally recommended anyway for better safety in case of mysqld crashes.

@enisoc enisoc merged commit 9b93e53 into vitessio:master May 21, 2016
@enisoc enisoc deleted the mysql57 branch May 21, 2016 05:56
Comment thread bootstrap.sh
@@ -154,14 +154,14 @@ if [ -z "$MYSQL_FLAVOR" ]; then
fi
case "$MYSQL_FLAVOR" in
"MySQL56")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not add a case for MySQL57 here?

"MySQL56" | "MySQL57")

In the current form it's misleading that test.go knows a "mysql57" flavor, but bootstrap.sh doesn't.

The entry in Dockerfile.mysql57 is also confusing because of that:

ENV MYSQL_FLAVOR MySQL56

Having a MySQL57 flavor would be cleaner. That we map it internally to 5.6 shouldn't bother the enduser.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In general I didn't want the "flavor" concept to be related to versions. The main point of flavors is to deal with forks of MySQL that diverge in significant ways, such as in GTID implementations. For example, there should be one flavor for Oracle's lineage and one for MariaDB since they've diverged to this extent.

The fact that the "MySQL56" flavor has a number in it is a mistake. I named it that because I thought "MySQL" would be too generic and I couldn't think of a better name - "OracleMySQL"?

The reason test.go distinguishes between 5.6 and 5.7 is because it needs to pick a specific Docker image, and we now maintain Docker images for multiple versions within a flavor. This is another mistake in naming; I shouldn't have called the test.go flag "flavor" because it's a different dimension than MYSQL_FLAVOR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

The test.go could be renamed to -image instead of -flavor for example?

After all, I'm fine with the current state and don't think we need to take any actions now.

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