anishgirianish commented on code in PR #62343: URL: https://github.com/apache/airflow/pull/62343#discussion_r2971838499
########## airflow-core/src/airflow/executors/local_executor.py: ########## Review Comment: > LocalExecutor is "one" of the executors means it can serve as template for other executors. But including K8s, Celery, Edge, AWS at least 4 additional exist in the repo. So the logic would need to be added to each. > > Technically - w/o looking into the code - I see 2 technical options: > > * Implement it in the base class (BaseExecutor) and all individual implementations can inherit > * Implement via a MixIn, then all Executors "just" need to use it... still all executor classes need to be adjusted > > Had only time to skim over the PR and have no answer, else I'd directly propose an alternative. But I worry a bit that complexity grows with this approach in each executor while I see in devlist additional features like https://lists.apache.org/thread/o0z8v01v9qq26r6qmvx8zwbkmho1fnbg might need to take the same route. Hi @jscheffl thank you very much the feedback. I just realzied, this was discussed with (@ferruzzi @jason810496 ) a bit before in this very pr disscussion as well and was actually planned as fast follow up to be using the mechanism following the pattern in this pr https://github.com/apache/airflow/pull/62645 to make it more generic. Please let me know your thoughts on this. thank you -- 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]
