[GitHub] [airflow] kaxil commented on pull request #8718: Set store_serialized_dags from config in UI

[GitBox <gi...@apache.org>]

kaxil commented on pull request #8718:
URL: https://github.com/apache/airflow/pull/8718#issuecomment-624163465


   >store_serialized_dags should be set to false everywhere except UI,
   experimental API and dag import
   
   It is, isn't it ? The DagBag has store_serialized_dags to False as default:
   
   https://github.com/apache/airflow/blob/master/airflow/models/dagbag.py#L90
   
   And this is only changed to True inside the UI, experimental API and when writing DAGs in the Scheduler.
   
   
   >migrations failed as they have used DagBag with SerializedDag before serialized_dags table was created
   
   Can you point me to the migration that failed? None of the migrations creates a DagBag that override the `DagBag.store_serialized_dags=False`
   
   
   >and, first of all, scheduler stopped parsing dags because
   DagBag::collect_dags method that it uses, returned no dags, as it does
   not collect dags when stored_serialized_dags is on.
   Generally, even though store_serialized_dags parameter is used in
   models and throughout common code, it should only be set to True in web
   server.
   
   Same as above `DagBag::collect_dags method` uses self.store_serialized_dags which is default to False and the Scheduler does not override this.
   
   I think most of the points you made in the PR Description rely on an assumption that DagBag.store_serialized_dags is set to True when Dag Serialization is turned on, which is not True.
   


----------------------------------------------------------------
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