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-3973] Commit after each alembic migration #4797

Merged
merged 1 commit into from
Mar 4, 2019
Merged

[AIRFLOW-3973] Commit after each alembic migration #4797

merged 1 commit into from
Mar 4, 2019

Conversation

eeshugerman
Copy link

Jira

https://issues.apache.org/jira/browse/AIRFLOW-3973

Description

If Variables are used in DAGs, and Postgres is used for the internal database, a fresh $ airflow initdb (or $ airflow resetdb) spams the logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate transaction.

See Jira ticket for more details.

I have tested this change with the default SQLite database and, of course, with Postgres.

Tests

No tests included as this is a one line change which adds no functionality whatsoever.

@eeshugerman eeshugerman changed the title [AIRFLOW-3973] problem: initdb spams log with errors | solution: run each migration in its own transaction [AIRFLOW-3973] Run each Alembic migration in its own transaction Feb 27, 2019
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.
@codecov-io
Copy link

Codecov Report

Merging #4797 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4797   +/-   ##
=======================================
  Coverage   74.44%   74.44%           
=======================================
  Files         450      450           
  Lines       28970    28970           
=======================================
  Hits        21566    21566           
  Misses       7404     7404

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 2ade912...ad67c68. Read the comment docs.

@eeshugerman eeshugerman changed the title [AIRFLOW-3973] Run each Alembic migration in its own transaction [AIRFLOW-3973] Run each Alembic migration in separate transaction Feb 28, 2019
@Fokko Fokko merged commit ea95e9c into apache:master Mar 4, 2019
@Fokko
Copy link
Contributor

Fokko commented Mar 4, 2019

Thanks @eeshugerman for figuring this out.

andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.
@eeshugerman
Copy link
Author

@Fokko Could we backport this to v1.10? Is it just a matter of opening a PR to v-1-10-test?

@Fokko
Copy link
Contributor

Fokko commented Jun 8, 2020

@eeshugerman please make a PR to the v1-10-test branch and I'll merge it.

@potiuk
Copy link
Member

potiuk commented Jun 8, 2020

Just a small update: I think merge PR should go to v1-10-stable - in test we are usually cherry-pick stuff (I have 23 cherry-picked commits to push soon) so better to keep it this way. 

@eeshugerman
Copy link
Author

eeshugerman commented Jun 8, 2020

A cherry-pick/rebase was required to get a clean PR to v1-10-stable or v1-10-test, so wasn't sure which to do? I opened a PR for each, feel free to close one or the other.

#9182
#9183

I'm assuming the failing PR is not up to date with master. Please rebase. check is not relevant here but let me know if I have the wrong idea.

@eeshugerman eeshugerman changed the title [AIRFLOW-3973] Run each Alembic migration in separate transaction [AIRFLOW-3973] Commit after each alembic migration Jun 9, 2020
@ashb ashb added this to the Airflow 1.10.11 milestone Jun 9, 2020
ashb pushed a commit that referenced this pull request Jun 9, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
kaxil pushed a commit that referenced this pull request Jun 14, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
ashb pushed a commit that referenced this pull request Jun 15, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
kaxil pushed a commit that referenced this pull request Jun 15, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
dimberman pushed a commit that referenced this pull request Jun 19, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
(cherry picked from commit 5b48a53)
potiuk pushed a commit that referenced this pull request Jun 29, 2020
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
If `Variable`s are used in DAGs, and Postgres is used for the internal
database, a fresh `$ airflow initdb` (or `$ airflow resetdb`) spams the
logs with error messages (but does not fail).

This commit corrects this by running each migration in a separate
transaction.

Co-authored-by: Elliott Shugerman <[email protected]>

(cherry picked from commit ea95e9c)
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