Fix: Mysql 5.7 id utf8mb3 #14535

hbrls · 2021-03-01T03:06:48Z

This may not be a ready PR. The code diff best expresses my ideas.

I'm initializing a mysql 5.7 db from empty using hub.docker.com, apache/airflow:2.0.1-python3.8. The migration scripts throw errors of (1071, 'Specified key was too long; max key length is 767 bytes'). AIRFLOW__CORE__SQL_ENGINE_COLLATION_FOR_IDS: utf8mb3_general_ci does not work. (ref #7570).

After go through the issues and code, I had an assumption that utf8mb3 config was not applied to all places that it should. I applied those migration scripts manually, then it works.

I'm not confident about how the migration scripts work.

Should it be generated by some tools and not modified manually?
Some scripts modify existed columns. Can I directly fix the create table scripts and skip the modify table ones? How does this affect the old users wants to upgrade?

~~Another question.~~

I cannot fix the line bop.create_primary_key('pk_xcom', ['dag_id', 'task_id', 'key', 'execution_date']). My DBA always told me not to use too many columns for keys. Why in this case dag_id and task_id is not enough?

EDIT: Fixed in master.

boring-cyborg · 2021-03-01T03:06:50Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

kurtqq · 2021-03-22T20:22:38Z

migration scripts are created with alembic.
you should change the target branch to Master.

hbrls · 2021-04-11T02:01:10Z

The docker image apache/airflow:2.0.1-python3.8 on dockerhub.com is quite behind master, which fixed some of the sql. Would you please release a newer tag?

uranusjr · 2021-04-11T05:28:19Z

There is a schedule to those; the next release will happen next week, if I understand correctly.

github-actions · 2021-05-27T00:12:10Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

hbrls · 2021-05-31T02:51:08Z

I'm working on the rebase to master. Sorry for false triggers.

potiuk

Can you please rebase it and fix my comments? I think this should be fine with utf8mb4

potiuk · 2021-05-31T15:21:53Z

airflow/migrations/versions/bbf4a7ad0465_remove_id_column_from_xcom.py

@@ -110,7 +110,8 @@ def upgrade():
            bop.drop_index('idx_xcom_dag_task_date')
            # mssql doesn't allow primary keys with nullable columns
            if conn.dialect.name != 'mssql':
-                bop.create_primary_key('pk_xcom', ['dag_id', 'task_id', 'key', 'execution_date'])
+                #bop.create_primary_key('pk_xcom', ['dag_id', 'task_id', 'key', 'execution_date'])
+                bop.create_primary_key('pk_xcom', ['dag_id', 'task_id', 'key'])


My mysql throws when creating the PK longer than 700. I have no idea how to fix it properly.

But since it's not related to utf8mb3，I will revert this line.

potiuk · 2021-05-31T15:22:16Z

airflow/migrations/versions/e3a246e0dc1_current_schema.py

@@ -38,6 +38,8 @@
 depends_on = None


+print(COLLATION_ARGS)


I guess we shoudl remove it ?

Yes, that's for debug. I will revert the line.

mik-laj · 2021-06-08T02:54:40Z

It looks like a bug. I reopened this PR.

github-actions · 2021-08-11T00:11:55Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

siddharthvp · 2021-08-15T06:13:22Z

airflow/migrations/versions/03afc6b6f902_increase_length_of_fab_ab_view_menu_.py

@@ -29,6 +29,8 @@
 from sqlalchemy.engine.reflection import Inspector

 # revision identifiers, used by Alembic.
+from airflow.models.base import COLLATION_ARGS


This line should go above the comment. In other files as well.

siddharthvp · 2021-08-21T05:46:19Z

Should this have been closed? This does indeed seem to be an issue and the patch would likely help with MariaDB compatibility as well (#16907).

potiuk · 2021-08-21T10:40:22Z

@hbrls - do you still want to rebase and merge this one? We are also just about to merge the #17729 to automatically default to utf8mb3 for MySQL (and mariadb) and I think it would be worthwile to add the collation to ids. or if not @siddharthvp maybe you would like to take that over and lead to completion?

hbrls · 2021-08-21T11:41:45Z

@potiuk I've fixed the comment position. And rebased to latest main.

github-actions · 2021-08-21T12:17:19Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2021-08-21T12:39:05Z

Some static check failed. Highly recommend installing pre-commits: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#pre-commit-hooks

potiuk · 2021-08-21T21:06:03Z

I think you still have static checks problem. Again, I hearltily recommend installing pre-commits and running them locally on your changes before pushing (if you install and use pre-commit it will happen automatically - you won't be able to commit until the tests pass. It saves a lot of time for iteration - for both you, and committers looking at your change)

hbrls · 2021-08-22T03:53:20Z

I've done the pre-commit except _build_images.sh. I guess it's ok now.

potiuk · 2021-08-22T20:29:42Z

Would you please rebase to latest main @hbrls ?

hbrls · 2021-08-23T01:16:18Z

Would you please rebase to latest main @hbrls ?

Done.

boring-cyborg · 2021-08-24T13:26:49Z

Awesome work, congrats on your first merged pull request!

hbrls force-pushed the fix-mysql-utf8mb3 branch from 22e8736 to 355a8fb Compare April 10, 2021 16:22

hbrls changed the base branch from v2-0-stable to master April 10, 2021 16:24

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 27, 2021

hbrls force-pushed the fix-mysql-utf8mb3 branch from 355a8fb to 5329ecf Compare May 31, 2021 02:43

hbrls requested review from ashb, dimberman, ephraimbuddy, jhtimmins, kaxil, mik-laj, potiuk, ryanahamilton, turbaszek, vikramkoka and XD-DENG as code owners May 31, 2021 02:43

hbrls force-pushed the fix-mysql-utf8mb3 branch 3 times, most recently from 987884b to 66f85dc Compare May 31, 2021 02:49

hbrls force-pushed the fix-mysql-utf8mb3 branch 4 times, most recently from be3a385 to ca910df Compare May 31, 2021 05:46

potiuk reviewed May 31, 2021

View reviewed changes

hbrls force-pushed the fix-mysql-utf8mb3 branch from ca910df to 7ef8f47 Compare June 1, 2021 02:16

mik-laj reopened this Jun 8, 2021

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jun 9, 2021

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 11, 2021

siddharthvp reviewed Aug 15, 2021

View reviewed changes

github-actions bot closed this Aug 21, 2021

potiuk reopened this Aug 21, 2021

hbrls force-pushed the fix-mysql-utf8mb3 branch from 7ef8f47 to c16bece Compare August 21, 2021 11:39

potiuk approved these changes Aug 21, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Aug 21, 2021

hbrls force-pushed the fix-mysql-utf8mb3 branch 3 times, most recently from 4bf517e to ccffe18 Compare August 21, 2021 15:25

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 22, 2021

hbrls force-pushed the fix-mysql-utf8mb3 branch from ccffe18 to 9fad619 Compare August 22, 2021 03:52

Fix: Mysql 5.7 id utf8mb3

dc96815

hbrls force-pushed the fix-mysql-utf8mb3 branch from 9fad619 to dc96815 Compare August 23, 2021 01:13

potiuk merged commit 5019916 into apache:main Aug 24, 2021

potiuk mentioned this pull request Jul 20, 2022

Airflow db init fails on mysql 5.7 #25144

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Mysql 5.7 id utf8mb3 #14535

Fix: Mysql 5.7 id utf8mb3 #14535

hbrls commented Mar 1, 2021 •

edited

Loading

boring-cyborg bot commented Mar 1, 2021

kurtqq commented Mar 22, 2021

hbrls commented Apr 11, 2021

uranusjr commented Apr 11, 2021

github-actions bot commented May 27, 2021

hbrls commented May 31, 2021

potiuk left a comment

potiuk May 31, 2021

hbrls Jun 1, 2021

potiuk May 31, 2021

hbrls Jun 1, 2021

mik-laj commented Jun 8, 2021

github-actions bot commented Aug 11, 2021

siddharthvp Aug 15, 2021

siddharthvp commented Aug 21, 2021

potiuk commented Aug 21, 2021

hbrls commented Aug 21, 2021

github-actions bot commented Aug 21, 2021

potiuk commented Aug 21, 2021

potiuk commented Aug 21, 2021

hbrls commented Aug 22, 2021

potiuk commented Aug 22, 2021

hbrls commented Aug 23, 2021

boring-cyborg bot commented Aug 24, 2021

Fix: Mysql 5.7 id utf8mb3 #14535

Fix: Mysql 5.7 id utf8mb3 #14535

Conversation

hbrls commented Mar 1, 2021 • edited Loading

boring-cyborg bot commented Mar 1, 2021

kurtqq commented Mar 22, 2021

hbrls commented Apr 11, 2021

uranusjr commented Apr 11, 2021

github-actions bot commented May 27, 2021

hbrls commented May 31, 2021

potiuk left a comment

Choose a reason for hiding this comment

potiuk May 31, 2021

Choose a reason for hiding this comment

hbrls Jun 1, 2021

Choose a reason for hiding this comment

potiuk May 31, 2021

Choose a reason for hiding this comment

hbrls Jun 1, 2021

Choose a reason for hiding this comment

mik-laj commented Jun 8, 2021

github-actions bot commented Aug 11, 2021

siddharthvp Aug 15, 2021

Choose a reason for hiding this comment

siddharthvp commented Aug 21, 2021

potiuk commented Aug 21, 2021

hbrls commented Aug 21, 2021

github-actions bot commented Aug 21, 2021

potiuk commented Aug 21, 2021

potiuk commented Aug 21, 2021

hbrls commented Aug 22, 2021

potiuk commented Aug 22, 2021

hbrls commented Aug 23, 2021

boring-cyborg bot commented Aug 24, 2021

hbrls commented Mar 1, 2021 •

edited

Loading