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

Fix dag run type enum query for mysqldb driver #13278

Merged
merged 5 commits into from
Jan 27, 2021
Merged

Conversation

houqp
Copy link
Member

@houqp houqp commented Dec 23, 2020

By default sqlalchemy pass query params as is to db dialect drivers for
query execution. This causes inconsistent behavior of query param
evaluation between different db drivers. For example, MySQLdb will
convert DagRunType.SCHEDULED to string 'DagRunType.SCHEDULED'
instead of string 'scheduled'.

To keep the behavior consistent across DB dialects, this patch
introduces a new enum aware sqlalchemy type based of
sqlalchemy.types.String to do the query param conversion within
sqlalchemy's ORM layer before passing the value down to individual DB
driver.

see #11621 for relevant
discussions.

airflow/utils/types.py Outdated Show resolved Hide resolved
ashb
ashb previously requested changes Dec 23, 2020
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

See comments

@ashb
Copy link
Member

ashb commented Dec 23, 2020

Why didn't mysql unit tests fail before?

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@houqp
Copy link
Member Author

houqp commented Dec 23, 2020

@ashb because when running the tests, run_type are being written as string 'DagRunType.SCHEDULED' into the db, so when it's queried using the same value, tests passed. We ran into this issue during upgrade because all of our old db rows have the column populated as string 'scheduled', which lead to crashes due to queries returning empty result.

All existing 1.x mysql users using mysqlclient driver will run into this issue after performing the 2.0 upgrade. New 2.0 mysql users might already have db populated with 'DagRunType.SCHEDULED', which needs manual fix up.

I will prepare a test case to guard against this regression going forward.

@ashb
Copy link
Member

ashb commented Dec 24, 2020

@ashb because when running the tests, run_type are being written as string 'DagRunType.SCHEDULED' into the db, so when it's queried using the same value, tests passed. We ran into this issue during upgrade because all of our old db rows have the column populated as string 'scheduled', which lead to crashes due to queries returning empty result.

All existing 1.x mysql users using mysqlclient driver will run into this issue after performing the 2.0 upgrade. New 2.0 mysql users might already have db populated with 'DagRunType.SCHEDULED', which needs manual fix up.

I will prepare a test case to guard against this regression going forward.

Oooh. Fuuu.

@ashb ashb added this to the Airflow 2.0.1 milestone Dec 24, 2020
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

(can't seem to comment without leaving a review anymore on Android app)

@houqp houqp requested a review from ashb December 24, 2020 08:34
@potiuk
Copy link
Member

potiuk commented Dec 24, 2020

All existing 1.x mysql users using mysqlclient driver will run into this issue after performing the 2.0 upgrade. New 2.0 mysql users might already have db populated with 'DagRunType.SCHEDULED', which needs manual fix up.

Not good :(

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

I am surprised that this slipped in alpha/beta and rcs

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

?

airflow/models/dagrun.py Outdated Show resolved Hide resolved
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@kaxil
Copy link
Member

kaxil commented Jan 25, 2021

Just rebased, let's merge this one once it passes CI

@kaxil kaxil dismissed ashb’s stale review January 27, 2021 21:43

Stale Review

@kaxil kaxil merged commit 53e8283 into apache:master Jan 27, 2021
kaxil pushed a commit that referenced this pull request Jan 27, 2021
By default sqlalchemy pass query params as is to db dialect drivers for
query execution. This causes inconsistent behavior of query param
evaluation between different db drivers. For example, MySQLdb will
convert `DagRunType.SCHEDULED` to string `'DagRunType.SCHEDULED'`
instead of string `'scheduled'`.

see #11621 for relevant
discussions.

(cherry picked from commit 53e8283)
kaxil pushed a commit that referenced this pull request Feb 4, 2021
By default sqlalchemy pass query params as is to db dialect drivers for
query execution. This causes inconsistent behavior of query param
evaluation between different db drivers. For example, MySQLdb will
convert `DagRunType.SCHEDULED` to string `'DagRunType.SCHEDULED'`
instead of string `'scheduled'`.

see #11621 for relevant
discussions.

(cherry picked from commit 53e8283)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Apr 12, 2021
By default sqlalchemy pass query params as is to db dialect drivers for
query execution. This causes inconsistent behavior of query param
evaluation between different db drivers. For example, MySQLdb will
convert `DagRunType.SCHEDULED` to string `'DagRunType.SCHEDULED'`
instead of string `'scheduled'`.

see apache#11621 for relevant
discussions.

(cherry picked from commit 53e8283)
(cherry picked from commit 4b1a6f7)
kaxil added a commit to astronomer/airflow that referenced this pull request Aug 28, 2021
same as apache#13278 but for `DagRunState` introduced in apache#16854

closes apache#17879
kaxil added a commit that referenced this pull request Aug 30, 2021
same as #13278 but for `DagRunState` introduced in #16854

closes #17879
kaxil added a commit that referenced this pull request Sep 15, 2021
same as #13278 but for `DagRunState` introduced in #16854

closes #17879

(cherry picked from commit a3f9c69)
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 27, 2021
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

(cherry picked from commit a3f9c690aa80d12ff1d5c42eaaff4fced07b9429)

GitOrigin-RevId: 66dcbf429aee0316e206a1d6ded089580fc94ddf
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 10, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2024
same as apache/airflow#13278 but for `DagRunState` introduced in apache/airflow#16854

closes apache/airflow#17879

GitOrigin-RevId: a3f9c690aa80d12ff1d5c42eaaff4fced07b9429
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.

5 participants