You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2024/08/02 04:34:56 UTC
[PR] Don't reserialize DAG if no revision's applied [airflow]
uranusjr opened a new pull request, #41207:
URL: https://github.com/apache/airflow/pull/41207
This saves some resource and helps avoid issues in dynamic DAG generation from the user.
Close #40082.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1701484518
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
Should we move it to inside "with create global lock? While it causes the lock to be applied for all such migrations - for a moment even if current revision is "current", it will have the other migrations to do even less work.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703767280
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
Agree. Likely not good idea.
But actually I think there is a problem with current solution. I think it does not work as intended - because when migration actually happens in another process, previous_revision will still be as it was when we started.
I think we will need to to just add:
```
previous_revision = current_revision = _get_current_revision(session=session)
```
right after qcquiring the lock.
At this point the database might already be migrated - if the migration happened in another processs - so both "previous revision" and "current revision" will be already different than before the lock.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703767280
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
Agree. Likely not good idea.
But sctually I think there is a problem with current solution. I think it does not work as intended - because when migration actually happens in another process, previous_revision will still be as it was when we started.
I think we will need to to just add:
```
previous_revision = current_revision = _get_current_revision(session=session)
```
right after qcquiring the lock.
At this point the database might already be migrated - if the migration happened in another processs - so both "previous revision" and "current revision" will be already different than before the lock.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703770448
##########
airflow/utils/db.py:
##########
@@ -1683,8 +1686,10 @@ def upgradedb(
os.environ["AIRFLOW__DATABASE__SQL_ALCHEMY_MAX_SIZE"] = val
settings.reconfigure_orm()
- if reserialize_dags:
+ current_revision = to_revision or _get_current_revision(session=session)
Review Comment:
Also - possibly reserialization should be also protected by the lock?
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703767280
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
Agree. Likely not good idea.
But sctually I think there is a problem with current solution. I think it does not work as intended - beuase when migration actually happens in another process, previous_revision will still be as it was when we started.
I think we will need to to just add:
```
previous_revision = current_revision = _get_current_revision(session=session)
```
right after qcquiring the lock.
At this point the database might already be migrated - if the migration happened in another processs - so both "previous revision" and "current revision" will be already different than before the lock.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703747365
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
That would imply also moving `initdb` into the block. I don’t have context for that—would it work?
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1703770448
##########
airflow/utils/db.py:
##########
@@ -1683,8 +1686,10 @@ def upgradedb(
os.environ["AIRFLOW__DATABASE__SQL_ALCHEMY_MAX_SIZE"] = val
settings.reconfigure_orm()
- if reserialize_dags:
+ current_revision = to_revision or _get_current_revision(session=session)
Review Comment:
Also - possibly reserializationshould be also protected by the lock?
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1704926615
##########
airflow/utils/db.py:
##########
@@ -1683,8 +1686,10 @@ def upgradedb(
os.environ["AIRFLOW__DATABASE__SQL_ALCHEMY_MAX_SIZE"] = val
settings.reconfigure_orm()
- if reserialize_dags:
+ current_revision = to_revision or _get_current_revision(session=session)
Review Comment:
In that case the next two lines probably should as well?
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #41207:
URL: https://github.com/apache/airflow/pull/41207
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1701484518
##########
airflow/utils/db.py:
##########
@@ -1660,11 +1660,14 @@ def upgradedb(
if errors_seen:
exit(1)
- if not to_revision and not _get_current_revision(session=session) and not use_migration_files:
+ current_revision = _get_current_revision(session=session)
Review Comment:
Should we move it to inside "with create global lock"? While it causes the lock to be applied for all such migrations - for a moment even if current revision is "current", it will have the other migrations to do even less work.
--
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
Re: [PR] Don't reserialize DAG if no revision's applied [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41207:
URL: https://github.com/apache/airflow/pull/41207#discussion_r1705037213
##########
airflow/utils/db.py:
##########
@@ -1683,8 +1686,10 @@ def upgradedb(
os.environ["AIRFLOW__DATABASE__SQL_ALCHEMY_MAX_SIZE"] = val
settings.reconfigure_orm()
- if reserialize_dags:
+ current_revision = to_revision or _get_current_revision(session=session)
Review Comment:
Yeah. I'd do that too likely.
--
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