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 typo in dag_schedule_dataset_alias_reference migration file #43314

Open
wants to merge 1 commit into
base: v2-10-test
Choose a base branch
from

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Oct 23, 2024

backport https://github.com/apache/airflow/pull/43245/files#diff-465bfca34e4030692c541248495b5abfe4deea606db126241dade041746fbd60


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@shahar1
Copy link
Contributor

shahar1 commented Oct 23, 2024

Static checks fail for some reason

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.

Hmm not so sure about just renaming constraints. What about folks who are already on this version?

@Lee-W
Copy link
Member Author

Lee-W commented Oct 24, 2024

Hmm not so sure about just renaming constraints. What about folks who are already on this version?

These reflect how the actual model is defined. As no feature is expected to be added to this version, I think these columns won't change in the future either.

@kaxil
Copy link
Member

kaxil commented Oct 24, 2024

Hmm not so sure about just renaming constraints. What about folks who are already on this version?

These reflect how the actual model is defined. As no feature is expected to be added to this version, I think these columns won't change in the future either.

If you download Airflow 2.8.0, run airflow db migrate, upgrade to 2.10.2 and run airflow db migrate again with Postgres or MySQL backend, the constraints are created with the old names. Example:

image

If we merge and ship this change, we will have some users with dsdar_dag_fkey as name of constraint and some on dsdar_dataset_fkey. In future, if we need to access a constraint in another migration via name that won't work for some users.

This is true for folks upgrading from let's say Airflow 2.8.0 to 2.10.2 --> for those folks tables are created from "migration" . It is only for new Airflow 2.10.0 user where all tables are just created from ORM.

image

@potiuk
Copy link
Member

potiuk commented Oct 24, 2024

Yep. Agree with @kaxil. That should be a separate migration. Also the original cahnge in main should be reverted and re-applied after that because people will go eventually 2.x -> 2.11 -> 3.x and things will break for them.

@Lee-W
Copy link
Member Author

Lee-W commented Oct 25, 2024

Got it. makes sense. I will create a fix pr to the main as well

Comment on lines +70 to +78
_rename_fk_constraint(
batch_op=batch_op,
original_name="dsdar_dataset_fkey",
new_name="dsdar_dataset_alias_fkey",
referent_table="dataset_alias",
local_cols=["alias_id"],
remote_cols=["id"],
ondelete="CASCADE",
)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we will have to do a little more here @Lee-W because we already have the case where the constraint is named differently for 2 different sets of users:

It is named

  1. dsdar_dataset_fkey : for 2.8.0 -> 2.10.2: because they are created from migration
  2. dsdar_dataset_alias_fkey: for new install 2.10.2: because they are created from model

So you will have to do similar if/else condition as you did on L85. Otherwise this might result in constraint not found error for folks with Case (2) above

@@ -224,7 +224,7 @@ class DagScheduleDatasetAliasReference(Base):
ForeignKeyConstraint(
columns=(dag_id,),
refcolumns=["dag.dag_id"],
name="dsdar_dag_fkey",
name="dsdar_dag_id_fkey",
Copy link
Member

@kaxil kaxil Oct 25, 2024

Choose a reason for hiding this comment

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

hmm? If the new name of constraint in migration is dsdar_dag_fkey, why are we changing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:db-migrations PRs with DB migration
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants