jscheffl commented on code in PR #59239: URL: https://github.com/apache/airflow/pull/59239#discussion_r2649712704
########## airflow-core/src/airflow/migrations/versions/0096_3_2_0_add_queue_column_to_trigger.py: ########## @@ -0,0 +1,50 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Add ``queue`` column to ``trigger`` table. + +Revision ID: c47f2e1ab9d4 +Revises: edc4f85a4619 +Create Date: 2025-12-09 20:30:40.500001 + +""" + +from __future__ import annotations + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "c47f2e1ab9d4" +down_revision = "edc4f85a4619" +branch_labels = None +depends_on = None +airflow_version = "3.2.0" + + +def upgrade(): + """Add ``queue`` column in trigger table.""" + with op.batch_alter_table("trigger") as batch_op: + batch_op.add_column(sa.Column("queue", sa.String(length=128), nullable=True)) Review Comment: in airflow-core/src/airflow/models/taskinstance.py:404 the queue field is defined to be 256, so should be the same in the triggerer ```suggestion batch_op.add_column(sa.Column("queue", sa.String(length=256), nullable=True)) ``` ########## airflow-core/src/airflow/migrations/versions/0015_2_9_0_update_trigger_kwargs_type.py: ########## @@ -55,16 +51,9 @@ def get_session() -> sa.orm.Session: def upgrade(): """Update trigger kwargs type to string and encrypt.""" with op.batch_alter_table("trigger") as batch_op: - batch_op.alter_column("kwargs", type_=sa.Text(), existing_nullable=False) - - if not context.is_offline_mode(): - session = get_session() - try: - for trigger in session.query(Trigger).options(lazyload(Trigger.task_instance)): - trigger.kwargs = trigger.kwargs - session.commit() - finally: - session.close() + batch_op.alter_column( + "kwargs", existing_type=ExtendedJSON(), type_=sa.Text(), existing_nullable=False + ) Review Comment: Okay, now I see the point of technical blocker here. But also still I am not convinced that fixing a previous migration will help because many many people will already have migrated from 2.9 to for example 3.1.5 and then further migrate. I think having existing triggerers still alive when migration is started might be a general problem. One radical thought if no other idea to fix: Can we drop support of migration from <2.10 when we release 3.2? To enforce people need to upgrate to 2.10 or 2.11 before getting to 3.2? Not sure what our backwards migration promise is, in the past we had cut for all <2.7. ########## providers/google/src/airflow/providers/google/cloud/triggers/bigquery.py: ########## Review Comment: Not sure why are the provider changes in the same PR? Smeels a bit unrelated. ########## providers/google/src/airflow/providers/google/cloud/triggers/dataproc.py: ########## Review Comment: Not sure why are the provider changes in the same PR? Smeels a bit unrelated. ########## airflow-core/src/airflow/models/trigger.py: ########## @@ -98,6 +98,7 @@ class Trigger(Base): encrypted_kwargs: Mapped[str] = mapped_column("kwargs", Text, nullable=False) created_date: Mapped[datetime.datetime] = mapped_column(UtcDateTime, nullable=False) triggerer_id: Mapped[int | None] = mapped_column(Integer, nullable=True) + queue: Mapped[str] = mapped_column(String(128), nullable=True) Review Comment: in airflow-core/src/airflow/models/taskinstance.py:404 the queue field is defined to be 256, so should be the same in the triggerer ```suggestion queue: Mapped[str] = mapped_column(String(256), nullable=True) ``` ########## providers/amazon/tests/unit/amazon/aws/operators/test_ecs.py: ########## Review Comment: Not sure why are the provider changes in the same PR? Smeels a bit unrelated. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
