You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/19 17:35:48 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #20962: Support generating SQL script for upgrades
ephraimbuddy opened a new pull request #20962:
URL: https://github.com/apache/airflow/pull/20962
This PR attempts to add support for generating sql scripts for upgrade.
Example command:
`airflow db upgrade e8d98d8ss99 --sql`
`airflow db upgrade --sql`
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792083778
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,18 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ _lowerband = revision.split(':')[0] if ":" in revision else None
+ if _lowerband and _lowerband != rev_2_0_0_head:
+ sql = True # Set to true if user passes a range of revisions
+ try:
+ command.history(config, rev_range=f"{rev_2_0_0_head}:{_lowerband}")
+ except Exception:
+ raise AirflowException(
+ f"Supported revision is from {rev_2_0_0_head} which is airflow 2.0.0 head"
+ )
Review comment:
This won't work if you try to have a revision range from (for example) 2.2.0->2.2.4 will it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1026690989
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r807915602
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
How about something as simple as:
```python
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}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1026731302
Hmm. Versions makes sense for end users, but revisions would make more sense for developers who want to inspect the migrations. Maybe we should support both?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r808116625
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
yes that's right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r804108700
##########
File path: airflow/utils/db.py
##########
@@ -1022,8 +1026,24 @@ def _validate_version_range(command, config, version_range):
'Please provide Airflow version range with the format "old_version:new_version"'
)
lower, upper = version_range.split(':')
+
if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
- raise AirflowException('Please provide valid Airflow versions that has migrations between them.')
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == 'No Migration' or REVISION_HEADS_MAP.get(upper) == 'No Migration':
Review comment:
Yeah. I think they should repeat the hash from 2.2.0 and the message should be only printed if hash(lower) == hash(upper).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r804025901
##########
File path: airflow/utils/db.py
##########
@@ -1022,8 +1026,24 @@ def _validate_version_range(command, config, version_range):
'Please provide Airflow version range with the format "old_version:new_version"'
)
lower, upper = version_range.split(':')
+
if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
- raise AirflowException('Please provide valid Airflow versions that has migrations between them.')
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == 'No Migration' or REVISION_HEADS_MAP.get(upper) == 'No Migration':
Review comment:
I'm not sure this is right. e.g. 2.2.1 -> 2.2.3 shouldn't show this message. Same with 2.1.2 -> 2.2.3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806756536
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
Should we use `-vr` here instead? since this is also about versions so that it doesn't confuse with revision-range?
##########
File path: airflow/utils/db.py
##########
@@ -989,8 +1005,93 @@ def _check_migration_errors(session: Session = NEW_SESSION) -> Iterable[str]:
session.commit()
+def _offline_migration(command, config, revision):
+ log.info("Running offline migrations for revision range %s", revision)
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ logging.disable(logging.CRITICAL)
+ command.upgrade(config, revision, sql=True)
+ logging.disable(logging.NOTSET)
+
+
+def _validate_version_range(command, config, version_range):
+ if ':' not in version_range:
+ raise AirflowException(
+ 'Please provide Airflow version range with the format "old_version:new_version"'
+ )
+ lower, upper = version_range.split(':')
+
+ if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == REVISION_HEADS_MAP.get(upper):
+ if sys.stdout.isatty():
+ size = os.get_terminal_size().columns
+ else:
+ size = 0
+ print(f"Hey this is your migration script from {lower}, to {upper}, but guess what?".center(size))
Review comment:
The message gives the impression of generating a migration script but there's no script at all for the user. It's a message to tell the user that there's no migration. I think it's Ok like this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806707192
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
```suggestion
("-r", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
```
`-v` is usually verbose or version, so lets avoid that confusion.
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
+)
+ARG_REVISION_RANGE = Arg(
+ ('-r', '--revision-range'),
Review comment:
This one is probably not going to be used very common, so we can remove the short opt here
```suggestion
('--revision-range',),
```
##########
File path: airflow/utils/db.py
##########
@@ -989,8 +1005,93 @@ def _check_migration_errors(session: Session = NEW_SESSION) -> Iterable[str]:
session.commit()
+def _offline_migration(command, config, revision):
+ log.info("Running offline migrations for revision range %s", revision)
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ logging.disable(logging.CRITICAL)
+ command.upgrade(config, revision, sql=True)
+ logging.disable(logging.NOTSET)
+
+
+def _validate_version_range(command, config, version_range):
+ if ':' not in version_range:
+ raise AirflowException(
+ 'Please provide Airflow version range with the format "old_version:new_version"'
+ )
+ lower, upper = version_range.split(':')
+
+ if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == REVISION_HEADS_MAP.get(upper):
+ if sys.stdout.isatty():
+ size = os.get_terminal_size().columns
+ else:
+ size = 0
+ print(f"Hey this is your migration script from {lower}, to {upper}, but guess what?".center(size))
Review comment:
Should we prefix all these messages with `--` to make them SQL comments?
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release if any.
Review comment:
```suggestion
- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
```
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
Short flags should be a single character. We could have this be `--range` (and keep the other as `--revision-range`) -- individual revisions are almost never going to be used outside of developers working on the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1026736613
> Hmm. Versions makes sense for end users, but revisions would make more sense for developers who want to inspect the migrations. Maybe we should support both?
Yeah. Agreed. This should be similar to "commit-ish" approach of Git where you can specify the commit ranges in various ways. Having "symbolic name" alongside "hash" sounds like good approach (later on we might want to add ^ and ~ and n-previous versions ;).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792457888
##########
File path: airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py
##########
@@ -26,48 +26,21 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import Column, Integer, String
-from sqlalchemy.ext.declarative import declarative_base
-
-from airflow.utils.sqlalchemy import UtcDateTime
# revision identifiers, used by Alembic.
revision = '8646922c8a04'
down_revision = '449b4072c2da'
branch_labels = None
depends_on = None
-Base = declarative_base()
-BATCH_SIZE = 5000
-
-
-class TaskInstance(Base): # type: ignore
- """Minimal model definition for migrations"""
-
- __tablename__ = "task_instance"
-
- task_id = Column(String(), primary_key=True)
- dag_id = Column(String(), primary_key=True)
- execution_date = Column(UtcDateTime, primary_key=True)
- pool_slots = Column(Integer, default=1)
-
def upgrade():
"""Change default pool_slots to 1 and make pool_slots not nullable"""
- connection = op.get_bind()
- sessionmaker = sa.orm.sessionmaker()
- session = sessionmaker(bind=connection)
-
- session.query(TaskInstance).filter(TaskInstance.pool_slots.is_(None)).update(
- {TaskInstance.pool_slots: 1}, synchronize_session=False
- )
- session.commit()
-
with op.batch_alter_table("task_instance", schema=None) as batch_op:
- batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False, server_default='1')
Review comment:
Should this use `text()` as well? (Honestly I’m not sure about the difference it makes.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #20962:
URL: https://github.com/apache/airflow/pull/20962
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r793840551
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,42 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ if ':' in revision:
+ sql = True
+ _lowerband, _upperband = revision.split(':')
+ head_to_lowerband_range = f"{rev_2_0_0_head}:{_lowerband}"
+ head_to_upperband_range = f"{rev_2_0_0_head}:{_upperband}"
+ for i in [head_to_lowerband_range, head_to_upperband_range]:
+ try:
+ command.history(config, rev_range=i)
+ except Exception:
+ raise AirflowException(
+ f"Error while checking history for revision range {i}. "
+ f"Check that {i.split(':')[1]} is a valid revision. "
+ f"Supported revision for offline migration is from {rev_2_0_0_head} "
+ f"which is airflow 2.0.0 head"
+ )
+ log.info("Running offline migrations for revision range %s", revision)
+ elif sql:
+ # user supplied a specific SQL revision
+ revision = f"{rev_2_0_0_head}:{revision}"
+ try:
+ command.history(config, rev_range=revision)
Review comment:
It produces output and I can't think of a way to silence it. There's no other way I could confirm that we should be good at producing the migration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792392628
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,18 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ _lowerband = revision.split(':')[0] if ":" in revision else None
+ if _lowerband and _lowerband != rev_2_0_0_head:
+ sql = True # Set to true if user passes a range of revisions
+ try:
+ command.history(config, rev_range=f"{rev_2_0_0_head}:{_lowerband}")
+ except Exception:
+ raise AirflowException(
+ f"Supported revision is from {rev_2_0_0_head} which is airflow 2.0.0 head"
+ )
Review comment:
Need to fix a case of giving a single revision that's not a range...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792476078
##########
File path: airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py
##########
@@ -26,48 +26,21 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import Column, Integer, String
-from sqlalchemy.ext.declarative import declarative_base
-
-from airflow.utils.sqlalchemy import UtcDateTime
# revision identifiers, used by Alembic.
revision = '8646922c8a04'
down_revision = '449b4072c2da'
branch_labels = None
depends_on = None
-Base = declarative_base()
-BATCH_SIZE = 5000
-
-
-class TaskInstance(Base): # type: ignore
- """Minimal model definition for migrations"""
-
- __tablename__ = "task_instance"
-
- task_id = Column(String(), primary_key=True)
- dag_id = Column(String(), primary_key=True)
- execution_date = Column(UtcDateTime, primary_key=True)
- pool_slots = Column(Integer, default=1)
-
def upgrade():
"""Change default pool_slots to 1 and make pool_slots not nullable"""
- connection = op.get_bind()
- sessionmaker = sa.orm.sessionmaker()
- session = sessionmaker(bind=connection)
-
- session.query(TaskInstance).filter(TaskInstance.pool_slots.is_(None)).update(
- {TaskInstance.pool_slots: 1}, synchronize_session=False
- )
- session.commit()
-
with op.batch_alter_table("task_instance", schema=None) as batch_op:
- batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False, server_default='1')
Review comment:
It works the same as using text. There's actually no need of using text since we are not giving it SQL expression https://alembic.sqlalchemy.org/en/latest/ops.html#alembic.operations.Operations.alter_column.params.server_default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r804025901
##########
File path: airflow/utils/db.py
##########
@@ -1022,8 +1026,24 @@ def _validate_version_range(command, config, version_range):
'Please provide Airflow version range with the format "old_version:new_version"'
)
lower, upper = version_range.split(':')
+
if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
- raise AirflowException('Please provide valid Airflow versions that has migrations between them.')
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == 'No Migration' or REVISION_HEADS_MAP.get(upper) == 'No Migration':
Review comment:
I'm not sure this is right. e.g. 2.2.1 -> 2.2.3 shouldn't show this message. Same with 2.1.2 -> 2.2.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r793284298
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,10 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_REVISION = Arg(("-r", "--revision"), default='heads', help="Revision target or range for --sql mode")
Review comment:
The `start:end` is correct. I made it so that if range is given in revision then automatically it's --sql mode. This is because range cannot be used in online mode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792194993
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,18 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ _lowerband = revision.split(':')[0] if ":" in revision else None
+ if _lowerband and _lowerband != rev_2_0_0_head:
+ sql = True # Set to true if user passes a range of revisions
+ try:
+ command.history(config, rev_range=f"{rev_2_0_0_head}:{_lowerband}")
+ except Exception:
+ raise AirflowException(
+ f"Supported revision is from {rev_2_0_0_head} which is airflow 2.0.0 head"
+ )
Review comment:
It will work. It will first check if we can get history from 2.0.0 to 2.2.0 which will succeed, then it means the range is correct and upgrade command can run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r794321480
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,42 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ if ':' in revision:
+ sql = True
+ _lowerband, _upperband = revision.split(':')
+ head_to_lowerband_range = f"{rev_2_0_0_head}:{_lowerband}"
+ head_to_upperband_range = f"{rev_2_0_0_head}:{_upperband}"
+ for i in [head_to_lowerband_range, head_to_upperband_range]:
+ try:
+ command.history(config, rev_range=i)
+ except Exception:
+ raise AirflowException(
+ f"Error while checking history for revision range {i}. "
+ f"Check that {i.split(':')[1]} is a valid revision. "
+ f"Supported revision for offline migration is from {rev_2_0_0_head} "
+ f"which is airflow 2.0.0 head"
+ )
+ log.info("Running offline migrations for revision range %s", revision)
+ elif sql:
+ # user supplied a specific SQL revision
+ revision = f"{rev_2_0_0_head}:{revision}"
+ try:
+ command.history(config, rev_range=revision)
Review comment:
I couldn't suppress the log. I think there's no problem if the log is there. Users would copy from BEGIN TRANSACTION. Apart from the history log, alembic also logs before the SQL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r794256824
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,42 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ if ':' in revision:
+ sql = True
+ _lowerband, _upperband = revision.split(':')
+ head_to_lowerband_range = f"{rev_2_0_0_head}:{_lowerband}"
+ head_to_upperband_range = f"{rev_2_0_0_head}:{_upperband}"
+ for i in [head_to_lowerband_range, head_to_upperband_range]:
+ try:
+ command.history(config, rev_range=i)
+ except Exception:
+ raise AirflowException(
+ f"Error while checking history for revision range {i}. "
+ f"Check that {i.split(':')[1]} is a valid revision. "
+ f"Supported revision for offline migration is from {rev_2_0_0_head} "
+ f"which is airflow 2.0.0 head"
+ )
+ log.info("Running offline migrations for revision range %s", revision)
+ elif sql:
+ # user supplied a specific SQL revision
+ revision = f"{rev_2_0_0_head}:{revision}"
+ try:
+ command.history(config, rev_range=revision)
Review comment:
Working on suppressing the logs..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r803247768
##########
File path: airflow/utils/db.py
##########
@@ -68,6 +69,17 @@
log = logging.getLogger(__name__)
+REVISION_HEADS_MAP = {
+ "2.0.0": "e959f08ac86c",
+ "2.0.1": "82b7c48c147f",
+ "2.0.2": "2e42bb497a22",
+ "2.1.0": "a13f7613ad25",
+ "2.1.3": "97cdd93827b8",
+ "2.1.4": "ccde3e26fe78",
+ "2.2.0": "7b2661a43ba3",
Review comment:
Should we include all releases here? If we pass `2.2.1`, users would get "Please provide valid Airflow versions that has migrations between them." right? I think it'd be worth adding them all so it "just works" from their perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r803356146
##########
File path: airflow/utils/db.py
##########
@@ -68,6 +69,17 @@
log = logging.getLogger(__name__)
+REVISION_HEADS_MAP = {
+ "2.0.0": "e959f08ac86c",
+ "2.0.1": "82b7c48c147f",
+ "2.0.2": "2e42bb497a22",
+ "2.1.0": "a13f7613ad25",
+ "2.1.3": "97cdd93827b8",
+ "2.1.4": "ccde3e26fe78",
+ "2.2.0": "7b2661a43ba3",
Review comment:
There's no migration head for that version unless we should use the head for a previous release that has migration but I think that a user using this feature is more likely to understand the message. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806756536
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
Should we use `-vr` here instead? since this is also about versions so that it doesn't confuse with revision-range?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r807341803
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-r", "--range"), help="Version range(start:end) for offline sql generation", default=None
+)
+ARG_REVISION_RANGE = Arg(
+ ('--revision-range',),
+ help='Revision range(start:end) to use for offline sql generation',
Review comment:
```suggestion
help='Migration revision range(start:end) to use for offline sql generation. Example: 'a13f7613ad25:7b2661a43ba3'',
```
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
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.
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-r", "--range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
```suggestion
("-r", "--range"), help="Version range(start:end) for offline sql generation. Example: '2.0.2:2.2.3'", default=None
```
##########
File path: airflow/models/taskinstance.py
##########
@@ -354,7 +354,7 @@ class TaskInstance(Base, LoggingMixin):
unixname = Column(String(1000))
job_id = Column(Integer)
pool = Column(String(256), nullable=False)
- pool_slots = Column(Integer, default=1, nullable=False)
+ pool_slots = Column(Integer, default=1, nullable=False, server_default=text("1"))
Review comment:
Why does it need `text("1")` instead of just `1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r808059432
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r793756208
##########
File path: airflow/utils/db.py
##########
@@ -1018,8 +1018,42 @@ def upgradedb(session: Session = NEW_SESSION):
exit(1)
with create_global_lock(session=session, lock=DBLocks.MIGRATIONS):
- log.info("Creating tables")
- command.upgrade(config, 'heads')
+ log.info("Running migrations")
+ rev_2_0_0_head = 'e959f08ac86c'
+ if ':' in revision:
+ sql = True
+ _lowerband, _upperband = revision.split(':')
+ head_to_lowerband_range = f"{rev_2_0_0_head}:{_lowerband}"
+ head_to_upperband_range = f"{rev_2_0_0_head}:{_upperband}"
+ for i in [head_to_lowerband_range, head_to_upperband_range]:
+ try:
+ command.history(config, rev_range=i)
+ except Exception:
+ raise AirflowException(
+ f"Error while checking history for revision range {i}. "
+ f"Check that {i.split(':')[1]} is a valid revision. "
+ f"Supported revision for offline migration is from {rev_2_0_0_head} "
+ f"which is airflow 2.0.0 head"
+ )
+ log.info("Running offline migrations for revision range %s", revision)
+ elif sql:
+ # user supplied a specific SQL revision
+ revision = f"{rev_2_0_0_head}:{revision}"
+ try:
+ command.history(config, rev_range=revision)
Review comment:
Doesn't this produce output? Is that intended?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792082814
##########
File path: airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py
##########
@@ -26,48 +26,21 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import Column, Integer, String
-from sqlalchemy.ext.declarative import declarative_base
-
-from airflow.utils.sqlalchemy import UtcDateTime
# revision identifiers, used by Alembic.
revision = '8646922c8a04'
down_revision = '449b4072c2da'
branch_labels = None
depends_on = None
-Base = declarative_base()
-BATCH_SIZE = 5000
-
-
-class TaskInstance(Base): # type: ignore
- """Minimal model definition for migrations"""
-
- __tablename__ = "task_instance"
-
- task_id = Column(String(), primary_key=True)
- dag_id = Column(String(), primary_key=True)
- execution_date = Column(UtcDateTime, primary_key=True)
- pool_slots = Column(Integer, default=1)
-
def upgrade():
"""Change default pool_slots to 1 and make pool_slots not nullable"""
- connection = op.get_bind()
- sessionmaker = sa.orm.sessionmaker()
- session = sessionmaker(bind=connection)
-
- session.query(TaskInstance).filter(TaskInstance.pool_slots.is_(None)).update(
- {TaskInstance.pool_slots: 1}, synchronize_session=False
- )
- session.commit()
-
with op.batch_alter_table("task_instance", schema=None) as batch_op:
- batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False, server_default='1')
Review comment:
Lol that's a lot easier :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1024002240
I will make a separate PR for SQL & MySQL. Both need the `copy_from` arg for every `batch ` https://alembic.sqlalchemy.org/en/latest/batch.html#working-in-offline-mode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r804108700
##########
File path: airflow/utils/db.py
##########
@@ -1022,8 +1026,24 @@ def _validate_version_range(command, config, version_range):
'Please provide Airflow version range with the format "old_version:new_version"'
)
lower, upper = version_range.split(':')
+
if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
- raise AirflowException('Please provide valid Airflow versions that has migrations between them.')
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == 'No Migration' or REVISION_HEADS_MAP.get(upper) == 'No Migration':
Review comment:
Yeah. I think they should repeat the hash from 2.2.0 and the "cat" message should be only printed if hash(lower) == hash(upper).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1040784802
Using github rebase button just messed my local branch up while fixing conflicts. It was a lot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r807028251
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
Short flags should be a single character. We could have this be `--range` (and keep the other as `--revision-range`) -- individual revisions are almost never going to be used outside of developers working on the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806758752
##########
File path: airflow/utils/db.py
##########
@@ -989,8 +1005,93 @@ def _check_migration_errors(session: Session = NEW_SESSION) -> Iterable[str]:
session.commit()
+def _offline_migration(command, config, revision):
+ log.info("Running offline migrations for revision range %s", revision)
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ logging.disable(logging.CRITICAL)
+ command.upgrade(config, revision, sql=True)
+ logging.disable(logging.NOTSET)
+
+
+def _validate_version_range(command, config, version_range):
+ if ':' not in version_range:
+ raise AirflowException(
+ 'Please provide Airflow version range with the format "old_version:new_version"'
+ )
+ lower, upper = version_range.split(':')
+
+ if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == REVISION_HEADS_MAP.get(upper):
+ if sys.stdout.isatty():
+ size = os.get_terminal_size().columns
+ else:
+ size = 0
+ print(f"Hey this is your migration script from {lower}, to {upper}, but guess what?".center(size))
Review comment:
The message gives the impression of generating a migration script but there's no script at all for the user. It's a message to tell the user that there's no migration. I think it's Ok like this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806709440
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release if any.
Review comment:
```suggestion
- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806707472
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
+)
+ARG_REVISION_RANGE = Arg(
+ ('-r', '--revision-range'),
Review comment:
This one is probably not going to be used very common, so we can remove the short opt here
```suggestion
('--revision-range',),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r807390685
##########
File path: airflow/models/taskinstance.py
##########
@@ -354,7 +354,7 @@ class TaskInstance(Base, LoggingMixin):
unixname = Column(String(1000))
job_id = Column(Integer)
pool = Column(String(256), nullable=False)
- pool_slots = Column(Integer, default=1, nullable=False)
+ pool_slots = Column(Integer, default=1, nullable=False, server_default=text("1"))
Review comment:
It doesn't really need the `text` but needs `"1"` and not `1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r807429788
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r803629659
##########
File path: airflow/utils/db.py
##########
@@ -68,6 +69,17 @@
log = logging.getLogger(__name__)
+REVISION_HEADS_MAP = {
+ "2.0.0": "e959f08ac86c",
+ "2.0.1": "82b7c48c147f",
+ "2.0.2": "2e42bb497a22",
+ "2.1.0": "a13f7613ad25",
+ "2.1.3": "97cdd93827b8",
+ "2.1.4": "ccde3e26fe78",
+ "2.2.0": "7b2661a43ba3",
Review comment:
I think we should add all versions. It's very easy to add another command which will filter out the versions that have the same migration head as the previous one if you want "miigration versions only".
And It also gives a great opportunity to print a "happy" message to the users when they run "get me the script to migrate from "2.2.1 to 2.2.2":
```
# Hey this is your migration script from 2.2.1, to 2.2.2, but guess what?
#
# There is no migration needed as the database has not changed between those versions. You are done.
#
# /\_/\
# (='_' )
# (, (") (")
#
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1019531633
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r793267992
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,10 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_REVISION = Arg(("-r", "--revision"), default='heads', help="Revision target or range for --sql mode")
Review comment:
```suggestion
ARG_REVISION = Arg(("-r", "--revision"), default='heads', help="Revision target or range (start:end) for --sql mode; ignored if --sql is not passed")
```
I think? Based on my understanding to the implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r808031952
##########
File path: dev/README_RELEASE_AIRFLOW.md
##########
@@ -205,6 +205,7 @@ The Release Candidate artifacts we vote upon should be the exact ones we vote ag
- Add a commit that updates `CHANGELOG.md` to add changes from previous version if it has not already added.
For now this is done manually, example run `git log --oneline v2-2-test..HEAD --pretty='format:- %s'` and categorize them.
- Add section for the release in `UPDATING.md`. If no new entries exist, put "No breaking changes" (e.g. `2.1.4`).
+- Update the `REVISION_HEADS_MAP` at airflow/utils/db.py to include the revision head of the release even if there are no migrations.
Review comment:
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 https://github.com/apache/airflow/pull/21601
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy edited a comment on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1024002240
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r792276176
##########
File path: airflow/migrations/versions/8646922c8a04_change_default_pool_slots_to_1.py
##########
@@ -26,48 +26,21 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import Column, Integer, String
-from sqlalchemy.ext.declarative import declarative_base
-
-from airflow.utils.sqlalchemy import UtcDateTime
# revision identifiers, used by Alembic.
revision = '8646922c8a04'
down_revision = '449b4072c2da'
branch_labels = None
depends_on = None
-Base = declarative_base()
-BATCH_SIZE = 5000
-
-
-class TaskInstance(Base): # type: ignore
- """Minimal model definition for migrations"""
-
- __tablename__ = "task_instance"
-
- task_id = Column(String(), primary_key=True)
- dag_id = Column(String(), primary_key=True)
- execution_date = Column(UtcDateTime, primary_key=True)
- pool_slots = Column(Integer, default=1)
-
def upgrade():
"""Change default pool_slots to 1 and make pool_slots not nullable"""
- connection = op.get_bind()
- sessionmaker = sa.orm.sessionmaker()
- session = sessionmaker(bind=connection)
-
- session.query(TaskInstance).filter(TaskInstance.pool_slots.is_(None)).update(
- {TaskInstance.pool_slots: 1}, synchronize_session=False
- )
- session.commit()
-
with op.batch_alter_table("task_instance", schema=None) as batch_op:
- batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column("pool_slots", existing_type=sa.Integer, nullable=False, server_default='1')
Review comment:
We should change TaskInstance declaration to match this. (The column on TaskInstance also has `default=1` but I’m not sure if it’s added in a later migration or was accidentally left out.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#issuecomment-1022027359
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806708729
##########
File path: airflow/utils/db.py
##########
@@ -989,8 +1005,93 @@ def _check_migration_errors(session: Session = NEW_SESSION) -> Iterable[str]:
session.commit()
+def _offline_migration(command, config, revision):
+ log.info("Running offline migrations for revision range %s", revision)
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore")
+ logging.disable(logging.CRITICAL)
+ command.upgrade(config, revision, sql=True)
+ logging.disable(logging.NOTSET)
+
+
+def _validate_version_range(command, config, version_range):
+ if ':' not in version_range:
+ raise AirflowException(
+ 'Please provide Airflow version range with the format "old_version:new_version"'
+ )
+ lower, upper = version_range.split(':')
+
+ if not REVISION_HEADS_MAP.get(lower) or not REVISION_HEADS_MAP.get(upper):
+ raise AirflowException('Please provide valid Airflow versions above 2.0.0.')
+ if REVISION_HEADS_MAP.get(lower) == REVISION_HEADS_MAP.get(upper):
+ if sys.stdout.isatty():
+ size = os.get_terminal_size().columns
+ else:
+ size = 0
+ print(f"Hey this is your migration script from {lower}, to {upper}, but guess what?".center(size))
Review comment:
Should we prefix all these messages with `--` to make them SQL comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20962: Support generating SQL script for upgrades
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20962:
URL: https://github.com/apache/airflow/pull/20962#discussion_r806707192
##########
File path: airflow/cli/cli_parser.py
##########
@@ -224,6 +224,16 @@ def _check(value):
default=ColorMode.AUTO,
)
+# DB args
+ARG_VERSION_RANGE = Arg(
+ ("-v", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
Review comment:
```suggestion
("-r", "--version-range"), help="Version range(start:end) for offline sql generation", default=None
```
`-v` is usually verbose or version, so lets avoid that confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org