[GitHub] [airflow] potiuk commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.

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

potiuk commented on issue #28276:
URL: https://github.com/apache/airflow/issues/28276#issuecomment-1368671353

   As dicussed in https://github.com/apache/airflow/pull/28288#discussion_r1057970596, I think this functionality should be split to different properties of executor - each of them with much narrower meaning but eventually we (or those who implemented their own executor) should be able to choose the right combination of flags for the perticular executor they implement.
   
   I am commenting here because I already merged #28288, but possibly the decision here might improve the merged code as well
   
   The "is_local" flag is rather meaningless and does not reflect it's true meaning, simply because there are multiple things "is_local" flag tried to achieve:
   
   * serving logs - > I think this should be a "serve_logs" flag. There are several reasons why the executor would like to serve logs, and while for example Local Executor should not do that by default, but LocalCeleryExecutor should for example (if we have it). I propose to use `serve_logs_by_worker` property for that one.
   
   * Supporting standalone airflow - another place where `is_local` is used is to determine fallback choice when standalone Airflow is used. There are two options here: a) we only suport LocalExecutor/SequentialExecutor in Standalone airflow and if any other executor is used, we fallback to one of those. Or we allowe to mark custom executor as supporting "Standalone" mode. I would be for the latter option and for adding `supports_standalone` property. This is more versatile and allows (in the future) to promote production-ready "standalone airflow" with custom executor - I see some interesting use cases for that one actually - you could have a fully standalone airflow with UI and scheduler and Triggerer, while doing a fully remote execution of tasks elsewhere. This is very appealing to have a much simpler deployment of Airflow - where you would just need `pip install airflow` and single `command` to run the whole of Airflow.
   
   * determine if we need to copy a configuration - there is this check:
   ```
         if executor_class.is_local:
             cfg_path = tmp_configuration_copy()
   ```
   
   Which I believe is only needed because of the impersonation - we need to copy a configuration file when backfilling. Since the backfill is done 
   
   And here as well I would propose to use new property `freeze_configuration_on_backfill` to control that one. I think there might be some executors that would like to do the same.
   
   I think if we split `is_local` flag to those three values, we should be able to determine the right set of properties that each executor should have.


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