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

Fix Connection.description migration for MySQL8 #12596

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Fix Connection.description migration for MySQL8 #12596

merged 1 commit into from
Nov 25, 2020

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Nov 24, 2020 •
edited by ashb
Loading

Due to a bug in Breeze initialization code, we were always running
against Postgres 9.6 and MySQL 5.7, even when the matrix selected
something else.

(We were overwriting the POSTGRES_VERSION and MYSQL_VERSION environment
variables in initialization code)

There was a connected problem that hidden the utf8mb4 problem as we
migrated to MySQL client 8, where utf8mb4 in mysql client 8
was silently falling back to latin 1 on database creation time
and we have not detected that.

We also fix uf8mb4 migration problem that was hidden due to those
missing tests.

Fixes #12588

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020

Trying to see if it works for both mysql 5.7 and 5.8

@potiuk potiuk requested review from ashb and kaxil November 24, 2020 18:19
@potiuk potiuk marked this pull request as draft November 24, 2020 18:19
@ashb
Copy link
Member

ashb commented Nov 24, 2020

Does this mean we can't store emoji in connection descriptions? ;)

@potiuk potiuk force-pushed the switch-to-utf8mb3-for-mysql branch from dfdfd1d to da85aa4 Compare November 24, 2020 18:39
@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020

Yeah . That's about it - and few really weird characters

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Nov 24, 2020
@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020 •
edited
Loading

I really think we should disable support for utf8mb4 for MySQL (we can check it at runtime).

I tested this version locally with both MySQL5.7 and MYSQL8.

Both 5.7 and 8 have utf8mb3 set as default when you choose utf8: https://dev.mysql.com/doc/refman/8.0/en/charset-unicode-utf8mb3.html. There is a note:

The utf8mb3 character set is deprecated and you should expect it to be removed in a future MySQL release. Please use utf8mb4 instead. Although utf8 is currently an alias for utf8mb3, at some point utf8 is expected to become a reference to utf8mb4. To avoid ambiguity about the meaning of utf8, consider specifying utf8mb4 explicitly for character set references instead of utf8.

This is a bit of a joke when you match it with the offending client's behavior 🤦

I think we never really supported Utf8mb4 and while I tried to do it with separate collation for some too-long-indexes in 5.7 it was rather weird.

WDYT?

@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020

For me it looks like hard statement on no utf8mb4 support at all.

@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020

BTW. The time when I looked at the uf8mb3 vs. utf8mb4 was when someone tried to use this in the description: 🦅

@ashb
Copy link
Member

ashb commented Nov 24, 2020

No idea, I haven't looked in to the details of mysql collations.

Do you know if switching to TEXT instead has any other advantages?

Going against mysql's docs seems like a bad idea?

@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020

No idea, I haven't looked in to the details of mysql collations.

Do you know if switching to TEXT instead has any other advantages?

Going against mysql's docs seems like a bad idea?

It's not going against it- we will likely have other "too big index" problems if we do that - just guessing. And it's not only this one field is a problem - in the same connection table there are already quite a few big test fields which are TEXT if you look at that.

@potiuk
Copy link
Member Author

potiuk commented Nov 24, 2020 •
edited
Loading

And it has another consequence - the migrations will have to be updated if we change the type of the colum - simply because for a short while during the migration the column will be added and fail. so we probably should somewhat adapt the old migrations to cope with it.

@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

Looking now. at the best way to fix the MySQL 8 problem

@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

@kaxil @ashb -> the simplest way of fixing the migration is indeed simply changing the field to Text. I am adding a fix for that and fixing two failing errors on mysql8 I see.

@potiuk potiuk force-pushed the switch-to-utf8mb3-for-mysql branch 2 times, most recently from 2dd7ee5 to 246f90b Compare November 25, 2020 09:20
@potiuk potiuk marked this pull request as ready for review November 25, 2020 09:20
@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

This change should address the migration @ashb and @kaxil - I made it in the way that the previous migration actually creates a Text field not string in case of mysql and the fix after that changes it to Text again. That should be safest if someone actually run the previous migration in non-UTF8mb4 mysql. I am looking at the two errors in MySQL8 that I saw in the previous run

@potiuk potiuk changed the title Switch to utf8mb3 for mysql Bring back MySQL 8 to test matrix Nov 25, 2020
@potiuk potiuk mentioned this pull request Nov 25, 2020
Closed
@potiuk potiuk force-pushed the switch-to-utf8mb3-for-mysql branch 2 times, most recently from c9d318a to 1573f04 Compare November 25, 2020 10:17
@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

I believe this have a chance of passing @ashb and @kaxil . Happy if you take a look.

@ashb ashb changed the title Bring back MySQL 8 to test matrix Fix Connection.description migration for MySQL8 Nov 25, 2020
@github-actions github-actions bot removed the full tests needed We need to run full set of tests for this PR to merge label Nov 25, 2020
@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

OK @ashb -> everything works for MySQL8 now.

@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020 •
edited
Loading

I will split out the the change and we can merge one after the other

@potiuk
Fix Connection.description migration for MySQL8
7853311 
Due to not executing MySQL8 tests Fixed in apache#12591 added
description for connection table was not compatible with
MySQL8 with utf8mb4 character set.

This change adds migration and fixes the previous migration
to make it compatible.
@potiuk potiuk force-pushed the switch-to-utf8mb3-for-mysql branch from 1573f04 to 7853311 Compare November 25, 2020 12:09
@potiuk
Copy link
Member Author

potiuk commented Nov 25, 2020

Split out now.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 25, 2020
@github-actions
Copy link

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@potiuk potiuk mentioned this pull request Nov 25, 2020
Merged
@ashb
Copy link
Member

ashb commented Nov 25, 2020

Good to merge now right?

@ashb ashb mentioned this pull request Nov 25, 2020
Merged
@potiuk potiuk merged commit cdaaff1 into apache:master Nov 25, 2020
@potiuk potiuk deleted the switch-to-utf8mb3-for-mysql branch November 25, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb approved these changes

@kaxil kaxil Awaiting requested review from kaxil

Assignees
No one assigned
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connection description migration breaks on MySQL 8
2 participants
@potiuk @ashb