Skip to content

Support generating SQL script for upgrades#20962

Merged
kaxil merged 3 commits into
apache:mainfrom
astronomer:add-sql-script
Feb 16, 2022
Merged

Support generating SQL script for upgrades#20962
kaxil merged 3 commits into
apache:mainfrom
astronomer:add-sql-script

Conversation

@ephraimbuddy

@ephraimbuddy ephraimbuddy commented Jan 19, 2022

Copy link
Copy Markdown
Contributor

This PR attempts to add support for generating sql scripts for upgrade.
Example command:
airflow db upgrade --revision-range e8d98d8ss99:78daisdu38d
airflow db upgrade --range 2.0.0:2.2.3


^ 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

potiuk commented Jan 23, 2022

Copy link
Copy Markdown
Member

I think this is quite needed. I actually find Alembic much worse and less intuitive than other solution (say Django migrations). So some tooling and possibly better instructions for those who attempt to add migrations is most welcome!

Comment thread airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py Outdated
Comment thread airflow/utils/db.py Outdated
Comment thread airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py Outdated
Comment thread airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py Outdated

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM except the server_default thing I don’t really understand. (SQLAlchemy is so complicated…)

@github-actions

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 26, 2022
Comment thread airflow/cli/cli_parser.py Outdated
Comment thread airflow/utils/db.py Outdated
@ephraimbuddy

ephraimbuddy commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

I will make a separate PR for SQLite & MSSQL. SQLite needs the copy_from arg for every batch operation. https://alembic.sqlalchemy.org/en/latest/batch.html#working-in-offline-mode
MSSQL custom codes for getting datetime column is an issue for offline mode

@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

I changed this to use version instead of revision.
Now we can do airflow db upgrade -v 2.0.0:2.2.3

cc: @uranusjr @ashb

@ashb ashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Few minor questions, looks good though.

@ephraimbuddy ephraimbuddy force-pushed the add-sql-script branch 4 times, most recently from 23d9185 to 4d8c336 Compare February 15, 2022 19:46
@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

Using github rebase button just messed my local branch up while fixing conflicts. It was a lot

@kaxil kaxil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we also add user-facing docs about it somewhere plz

Comment thread airflow/cli/cli_parser.py Outdated
Comment thread airflow/cli/cli_parser.py Outdated
Comment thread airflow/models/taskinstance.py Outdated
Comment thread dev/README_RELEASE_AIRFLOW.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should have a pre-commit to verify that a tuple containing the current Airflow version is added to REVISION_HEADS_MAP to avoid forgetting it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like a pre-commit won't be possible in this case as we only update the migration head when there's a new airflow release.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about something as simple as:

from airflow.utils.db import REVISION_HEADS_MAP
from airflow.version import version as airflow_version
from packaging.version import Version


if not Version(airflow_version).is_prerelease and airflow_version not in REVISION_HEADS_MAP:
    raise Exception(f"Airflow version {airflow_version} is not in the revision map {REVISION_HEADS_MAP}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i was also thinking about this issue. i saw that we had a table in the docs which contains this mapping. so what i was thinking to do was store that table in yaml form, and it could be used to generate the docs table and to support CLI commands which need to map version to revision, and that's what i do in #21601

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks good, I think the challenge would be to get the migration heads of the versions and discard every other migration in-between when going from version to version. It's still necessary to have a pre-commit or a ci job that makes sure we have the migration head updated before a release

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes that's right.

This PR attempts to add support for generating sql scripts for upgrade.
Example command:
`airflow db upgrade e8d98d8ss99 --sql`
`airflow db upgrade --sql`

Support offline mode from 2.0.0 upwards

Add tests

fixup! Add tests

fixup! fixup! Add tests

add server default to pool_slots

Fix single digit revision in offline mode to start from 2.0.0 head

fixup! Fix single digit revision in offline mode to start from 2.0.0 head

Apply suggestions from code review

Separate offline code from online to reduce log noise

fixup! Separate offline code from online to reduce log noise

Use version instead of revision

fixup! Use version instead of revision

add back revision range

fixup! add back revision range

Add instruction to update revision map during release

Exclude offline migration for sqlite and mssql less than 2.2.0

Use version range instead of just version in cli parser

print nice message for versions without migrations

fixup! print nice message for versions without migrations

Support generating SQL script for upgrades

This PR attempts to add support for generating sql scripts for upgrade.
Example command:
`airflow db upgrade e8d98d8ss99 --sql`
`airflow db upgrade --sql`

Support offline mode from 2.0.0 upwards

Add tests

fixup! Add tests

fixup! fixup! Add tests

Fix single digit revision in offline mode to start from 2.0.0 head

fixup! Fix single digit revision in offline mode to start from 2.0.0 head

Separate offline code from online to reduce log noise

Use version instead of revision

print nice message for versions without migrations

fixup! print nice message for versions without migrations

Apply suggestions from code review

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>

use range instead of version-range

Fix test

Support generating SQL script for upgrades

This PR attempts to add support for generating sql scripts for upgrade.
Example command:
`airflow db upgrade e8d98d8ss99 --sql`
`airflow db upgrade --sql`

Support offline mode from 2.0.0 upwards

Add tests

fixup! Add tests

fixup! fixup! Add tests

Fix single digit revision in offline mode to start from 2.0.0 head

fixup! Fix single digit revision in offline mode to start from 2.0.0 head

Separate offline code from online to reduce log noise

Use version instead of revision

add back revision range

Use version range instead of just version in cli parser

print nice message for versions without migrations

fixup! print nice message for versions without migrations

Support generating SQL script for upgrades

This PR attempts to add support for generating sql scripts for upgrade.
Example command:
`airflow db upgrade e8d98d8ss99 --sql`
`airflow db upgrade --sql`

Support offline mode from 2.0.0 upwards

Add tests

fixup! Add tests

fixup! fixup! Add tests

Fix single digit revision in offline mode to start from 2.0.0 head

fixup! Fix single digit revision in offline mode to start from 2.0.0 head

Separate offline code from online to reduce log noise

Use version instead of revision

print nice message for versions without migrations

fixup! print nice message for versions without migrations

Apply suggestions from code review

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>

Apply suggestions from code review

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>

fix conflict
@ephraimbuddy ephraimbuddy requested a review from kaxil February 16, 2022 10:52

@kaxil kaxil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving it so it is not blocked for adding pre-commit - that can be a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI full tests needed We need to run full set of tests for this PR to merge type:new-feature Changelog: New Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants