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 argiumets 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
