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

[AIRFLOW-7058] Add support for different DB versions #7717

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 13, 2020

Please review only the last commit - it is based on #7570


Issue link: AIRFLOW-7058

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.
Read the Pull Request Guidelines for more information.

@potiuk
Copy link
Member Author

potiuk commented Mar 13, 2020

Let's see if it works with MySQL 8 :)

@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch from 739cda0 to c74d0ea Compare March 13, 2020 18:08
@ashb
Copy link
Member

ashb commented Mar 13, 2020

Last time I looked at mysql 8 we failed cos of some bulk export type thing we were using

@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch 3 times, most recently from 882eb58 to e754c77 Compare March 14, 2020 10:42
@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch from e754c77 to 411e850 Compare March 14, 2020 11:49
@codecov-io
Copy link

codecov-io commented Mar 14, 2020

Codecov Report

Merging #7717 into master will decrease coverage by 80.69%.
The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #7717       +/-   ##
==========================================
- Coverage   86.92%   6.22%   -80.70%     
==========================================
  Files         915     914        -1     
  Lines       44152   44145        -7     
==========================================
- Hits        38377    2747    -35630     
- Misses       5775   41398    +35623     
Impacted Files Coverage Δ
airflow/models/base.py 93.75% <83.33%> (-6.25%) ⬇️
airflow/models/log.py 48.48% <100.00%> (-51.52%) ⬇️
airflow/models/slamiss.py 93.33% <100.00%> (ø)
airflow/models/taskfail.py 65.21% <100.00%> (-34.79%) ⬇️
airflow/models/taskinstance.py 22.79% <100.00%> (-72.19%) ⬇️
airflow/models/taskreschedule.py 67.74% <100.00%> (-32.26%) ⬇️
airflow/models/xcom.py 43.95% <100.00%> (-37.37%) ⬇️
airflow/www/forms.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/utils/json.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/www/widgets.py 0.00% <0.00%> (-100.00%) ⬇️
... and 849 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e31e9dd...766d745. Read the comment docs.

@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch from 411e850 to 89fa27f Compare March 14, 2020 11:56
@potiuk
Copy link
Member Author

potiuk commented Mar 14, 2020

@ashb @kaxil - I've added description to README to make it clear which Python/MySQL/Postgres DB versions are currently tested with Airflow. Several people asked recently as they could not find this information :)

@potiuk
Copy link
Member Author

potiuk commented Mar 14, 2020

Last time I looked at mysql 8 we failed cos of some bulk export type thing we were using

I will try both python 3.8 and mysql 8 as separate PRs :)

@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch from 89fa27f to 2446789 Compare March 14, 2020 12:26
@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch 2 times, most recently from 766d745 to 0afc92d Compare March 14, 2020 19:24
@potiuk potiuk force-pushed the AIRFLOW-7058-db-versions branch from 0afc92d to e30a850 Compare March 14, 2020 22:09
@potiuk potiuk merged commit 271ee64 into apache:master Mar 14, 2020
potiuk added a commit that referenced this pull request Mar 15, 2020
kaxil pushed a commit that referenced this pull request Mar 19, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 19, 2020
potiuk added a commit to PolideaInternal/airflow that referenced this pull request Nov 25, 2020
We missed that when we added support
for differnet mysql versions in apache#7717 when we removed default
character set setting for the database server.

This change forces the default on database server to be
utf8mb4 - regardless if MySQL 5.7 or MySQL8 is used.
Utf8mb4 is default for MySQL8 but latin1 is default fo MySQL 5.7.

There was a suspected root cause of the problem:

https://dev.mysql.com/doc/refman/8.0/en/charset-connection.html
where mysql client falls back to the default collation if
the client8 is used with 5.7 database, but this should be
no problem if the default DB character set is forced to be
utf8mb4

This PR restores forcing the server-side encoding.
potiuk added a commit that referenced this pull request Nov 25, 2020
* Fix Connection.description migration for MySQL8

Due to not executing MySQL8 tests Fixed in #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.

* Fixes inconsistent setting of encoding on Mysql 5.7/8

We missed that when we added support
for differnet mysql versions in #7717 when we removed default
character set setting for the database server.

This change forces the default on database server to be
utf8mb4 - regardless if MySQL 5.7 or MySQL8 is used.
Utf8mb4 is default for MySQL8 but latin1 is default fo MySQL 5.7.

There was a suspected root cause of the problem:

https://dev.mysql.com/doc/refman/8.0/en/charset-connection.html
where mysql client falls back to the default collation if
the client8 is used with 5.7 database, but this should be
no problem if the default DB character set is forced to be
utf8mb4

This PR restores forcing the server-side encoding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants