o-nikolas commented on PR #31001: URL: https://github.com/apache/airflow/pull/31001#issuecomment-1546080768
> Loooks cool!. > > I think we should discuss it together with the #30727 and decide - if they are both needed to complete other tasks, I think this one has smaller potential for disruptions - k8s part has far more potential for conflicts and fixes - even yesterday I saw an issue that could have require k8s changes. > > If those two willl help with the final steps of AIP-51, I am cool with merging both and biting the cherry-picking bullet. Yeah, I still think we cannot finish/merge #29055 without these changes (see [here](https://github.com/apache/airflow/pull/29055#issuecomment-1479968269)). The Airflow CLI code will see too much of a performance regression. > > But the liveness/health checks need a bit more work due to back-compatibilty, > > There are two things to be done: > > 1. I think the liveness check in Helm chart shoudl be airflow version dependent. Very similar to what we have for scheduler/triggerer: > > Our Helm Chart should be able to be used with past versions of Airflow and this change will make it to stop working for version < 2.7.0, so we need to have similar conditional template for the liveness checks. Yupp, that's fair, I can look into this. > Another option would be to change the liveness check to a new command (similarly to what have been done in those other liveness checks. Rather than calling the `exec /entrypoint python -m celery --app airflow.executors.celery_executor_utils.app inspect ping -d celery@$(hostname` we could add an airflow command that would do that (`airflow celery check --local`) - with hostname option as well - learning from the experience when we introduced the "jobs check` command. > > I think the second option here is really something we should do (and that's why this one is also I thin rather candidate for 2.7.0 than say 2.6.2) - because it will add "new feature". If we go this route, I believe this should be done in another PR, so that this one shouldn't have too much novel code changes and just contain the refactoring. > > 2. The second piece of compatibility is what should happen if someone uses the old Helm Chart (or old docker-compose) for newer airflow versions with. That one shoudl also work with deprecation likely or at the very least should tell the user what to do. I would be for the former. > > > And it should be possible with PEP-563 and dynamic attributes (#26290) we should be able to add `app` attribute dynamically in `airflow.executors.celery_executor` module - in the way that it will not be auto-completeable and visible, but if someone will continue using it, it could raise a deprecation warning but it will continue to work, importing the "utils" one instead dynamically. Interesting, I'll have to look into this. As long as it does not slow down the import time through the regular code path this should be fine. > > I think we shoudl solve both problems, :+1: -- 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]
