You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/07 10:41:31 UTC
[GitHub] [airflow] anitakar opened a new pull request #8764: Store dags cleanup 1 10
anitakar opened a new pull request #8764:
URL: https://github.com/apache/airflow/pull/8764
---
Make sure to mark the boxes below before creating PR: [x]
- [ ] Description above provides context of the change
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Target Github ISSUE in description if exists
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] anitakar commented on a change in pull request #8764: Store dags cleanup 1 10
Posted by GitBox <gi...@apache.org>.
anitakar commented on a change in pull request #8764:
URL: https://github.com/apache/airflow/pull/8764#discussion_r421691284
##########
File path: airflow/www/views.py
##########
@@ -94,7 +94,16 @@
UTF8_READER = codecs.getreader('utf-8')
-dagbag = models.DagBag(settings.DAGS_FOLDER, store_serialized_dags=STORE_SERIALIZED_DAGS)
+try:
+ async_dagbag_loader = conf.getboolean('webserver', 'async_dagbag_loader')
Review comment:
Sorry for that. I thought it got lost or was removed. It is not necessary with dag serialization and definitely such big functionalities should be first discussed via. AIP
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #8764: Store dags cleanup 1 10
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8764:
URL: https://github.com/apache/airflow/pull/8764#discussion_r421470122
##########
File path: airflow/www_rbac/views.py
##########
@@ -564,7 +568,8 @@ def dag_details(self, session=None):
dag_id = request.args.get('dag_id')
dag_orm = DagModel.get_dagmodel(dag_id, session=session)
# FIXME: items needed for this view should move to the database
- dag = dag_orm.get_dag(STORE_SERIALIZED_DAGS)
+ dag = dag_orm.get_dag(
+ conf.getboolean('core', 'store_serialized_dags', fallback=False))
Review comment:
No. This does not make sense.
Thsi change just makes it slower. STORE_SERIALIZED_DAGS is `conf.getboolean('core', 'store_serialized_dags', fallback=False))`, but without calling the slow conf.getboolean every time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #8764: Store dags cleanup 1 10
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8764:
URL: https://github.com/apache/airflow/pull/8764#discussion_r421471104
##########
File path: airflow/www/views.py
##########
@@ -94,7 +94,16 @@
UTF8_READER = codecs.getreader('utf-8')
-dagbag = models.DagBag(settings.DAGS_FOLDER, store_serialized_dags=STORE_SERIALIZED_DAGS)
+try:
+ async_dagbag_loader = conf.getboolean('webserver', 'async_dagbag_loader')
Review comment:
This is not a "clean-up" -- it is a whole new feature.
And it was a feature that was discussed and rejected in the original serialized DAG proposal/PR. Please to not bring it back in without a very clear indication of what problem it solves over the current method against 1.10.10.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #8764: Store dags cleanup 1 10
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #8764:
URL: https://github.com/apache/airflow/pull/8764#issuecomment-625232205
I'm going to close this, as the main feature here is not needed with Dag serialization on, and the other changes are a performance regression.
We can re-open this is you disagree.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] anitakar commented on a change in pull request #8764: Store dags cleanup 1 10
Posted by GitBox <gi...@apache.org>.
anitakar commented on a change in pull request #8764:
URL: https://github.com/apache/airflow/pull/8764#discussion_r421691736
##########
File path: airflow/www_rbac/views.py
##########
@@ -564,7 +568,8 @@ def dag_details(self, session=None):
dag_id = request.args.get('dag_id')
dag_orm = DagModel.get_dagmodel(dag_id, session=session)
# FIXME: items needed for this view should move to the database
- dag = dag_orm.get_dag(STORE_SERIALIZED_DAGS)
+ dag = dag_orm.get_dag(
+ conf.getboolean('core', 'store_serialized_dags', fallback=False))
Review comment:
Sure. Airflow does not support dynamic settings. And even if it did, it should not be one of them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org