potiuk commented on code in PR #35868: URL: https://github.com/apache/airflow/pull/35868#discussion_r1407674731
########## airflow/migrations/versions/0027_1_10_0_add_time_zone_awareness.py: ########## @@ -139,11 +139,11 @@ def upgrade(): type_=mysql.TIMESTAMP(fsp=6), ) else: - # sqlite and mssql datetime are fine as is. Therefore, not converting Review Comment: I am not really sure if we want to remove mssql in all the migrations from the past. Maybe yes, Maybe no. I have arguments for both. ## The argumets for "removing MSSQL From migrations": I am leaning towards removing them (so basically what this PR does), but I am a bit torn on that one. On one hand I would love to constraint and limit the "mssql" users to only use Airflow 2.7.3 and never, ever use airflow >= 2.8.0 for anything MSSQL. On the other hand it feels a little weird. I'd love other's opinion - especially those who worked on the migrations in the past: @ephraimbuddy @dstandish @ashb @kaxil @eladkal ## The argument for "leaving MSSQL in migrations": It does not really looks like it is necessary to remove them and does not cost us much. It's a bit sunken cost already and we are not going to maintain those migrations anyway. <aybe at some point in time we might want to merge them somehow and get a new "starting point" migration in the future, but I am not sure what is the value in removing mssql from the old migrations. It feels a bit weird to change the past migrations that are already marked as "I run this migration already" by alembic. I know it SHOULD NOT matter when you have Postgres/MySQL, but it feels a bit wrong to modify the migration scripts, especially that it does not hurt us to leave them there (we generally do not touch/maintain migrations too often). I see two potential prolems with removing the migrations for mssql from those migrations: * there is a risk we remove something here accidentally by missing it in review - there are a lot of files to be reviewed and lots of conditions, there is a slight risk we will miss it. But with careful review we can mitigate it. * there is a side-effect on --from-ref/--to-ref, --from-version/--to-version. Since we are still supporting MSSQL for airlfow < 2.8.0, it would be nice if Airflow 2.8+ still supports migrations for older versions of airflow even for MSSQL. I think (but I might be wrong) - just leaving the mssql in migration code will just allow it to be run and it SHOULD work in general. So users will still be able - for example - to run `airflow db migrate --to-version 2.7.3` using Airflow 2.8, 2.9 etc. There will be no need to run those migration with `Airflow 2.7.3` strictly. ## My current thinking But I am bit torn on that one. It also relates to the discussion we had on https://github.com/apache/airflow/pull/35861 and what we want to tell our users who run MSSQL now. So far my points there at least were: * let's constraint the set of tool and environment as much as we can to reduce variability of environment * let's provide a tool/script that they can use - but also modify if needed, so that it's not part of "airflow" itself So if we follow and agree that line of thought - then yes, removing MSSQL from Airflow 2.8.0 and not allowing the MSSQL users to run `--to-version 2.7.3` feels about right. On the othere hand - if we allow for variability, and letting the users to do the migrations in various ways, it's ok to leave it in. I wonder what you think? ########## airflow/migrations/versions/0027_1_10_0_add_time_zone_awareness.py: ########## @@ -139,11 +139,11 @@ def upgrade(): type_=mysql.TIMESTAMP(fsp=6), ) else: - # sqlite and mssql datetime are fine as is. Therefore, not converting Review Comment: I am not really sure if we want to remove mssql in all the migrations from the past. Maybe yes, Maybe no. I have arguments for both. ## The arguments for "removing MSSQL From migrations": I am leaning towards removing them (so basically what this PR does), but I am a bit torn on that one. On one hand I would love to constraint and limit the "mssql" users to only use Airflow 2.7.3 and never, ever use airflow >= 2.8.0 for anything MSSQL. On the other hand it feels a little weird. I'd love other's opinion - especially those who worked on the migrations in the past: @ephraimbuddy @dstandish @ashb @kaxil @eladkal ## The argument for "leaving MSSQL in migrations": It does not really looks like it is necessary to remove them and does not cost us much. It's a bit sunken cost already and we are not going to maintain those migrations anyway. <aybe at some point in time we might want to merge them somehow and get a new "starting point" migration in the future, but I am not sure what is the value in removing mssql from the old migrations. It feels a bit weird to change the past migrations that are already marked as "I run this migration already" by alembic. I know it SHOULD NOT matter when you have Postgres/MySQL, but it feels a bit wrong to modify the migration scripts, especially that it does not hurt us to leave them there (we generally do not touch/maintain migrations too often). I see two potential prolems with removing the migrations for mssql from those migrations: * there is a risk we remove something here accidentally by missing it in review - there are a lot of files to be reviewed and lots of conditions, there is a slight risk we will miss it. But with careful review we can mitigate it. * there is a side-effect on --from-ref/--to-ref, --from-version/--to-version. Since we are still supporting MSSQL for airlfow < 2.8.0, it would be nice if Airflow 2.8+ still supports migrations for older versions of airflow even for MSSQL. I think (but I might be wrong) - just leaving the mssql in migration code will just allow it to be run and it SHOULD work in general. So users will still be able - for example - to run `airflow db migrate --to-version 2.7.3` using Airflow 2.8, 2.9 etc. There will be no need to run those migration with `Airflow 2.7.3` strictly. ## My current thinking But I am bit torn on that one. It also relates to the discussion we had on https://github.com/apache/airflow/pull/35861 and what we want to tell our users who run MSSQL now. So far my points there at least were: * let's constraint the set of tool and environment as much as we can to reduce variability of environment * let's provide a tool/script that they can use - but also modify if needed, so that it's not part of "airflow" itself So if we follow and agree that line of thought - then yes, removing MSSQL from Airflow 2.8.0 and not allowing the MSSQL users to run `--to-version 2.7.3` feels about right. On the othere hand - if we allow for variability, and letting the users to do the migrations in various ways, it's ok to leave it in. I wonder what you think? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org