Skip to content

Conversation

@alexchen8
Copy link
Contributor

@alexchen8 alexchen8 commented Feb 6, 2022

Replacing existing celery subclass check in celery cli validation with a new attribute to Celery based executors.
Also moved celery module check a level higher. IMO, it is worth checking even if the user is using built-in Celery or C-K executors.

The reason behind this change is to allow any custom executor to utilize built-in airflow celery executor/worker. There are some cases that doesn't make sense to subclass the existing Celery / C-K executors (for example a similar implementation of C-K executor that has some other executors for routing, but not Kubernetes").

This change should be backward compatible, assuming subclasses of C/C-K in the wild does not have supports_celery attribute defined for other use.

@alexchen8 alexchen8 force-pushed the executor-supports-celery branch 2 times, most recently from 4ebe83c to 48636a6 Compare February 6, 2022 00:39
@boring-cyborg boring-cyborg bot added area:CLI provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:Scheduler including HA (high availability) scheduler labels Feb 6, 2022
@alexchen8 alexchen8 force-pushed the executor-supports-celery branch 3 times, most recently from 795c70f to fd4f1bf Compare February 7, 2022 17:42
@alexchen8
Copy link
Contributor Author

@ashb Just want to see what's your thought on this as the last modification was from you.

@alexchen8
Copy link
Contributor Author

Just want to resurface this PR... @potiuk Do you have any thoughts on this?

@alexchen8 alexchen8 force-pushed the executor-supports-celery branch from 244b1a6 to 8820deb Compare February 15, 2022 20:10
@potiuk
Copy link
Member

potiuk commented Feb 19, 2022

I will need one more committer's approval though.

@alexchen8 alexchen8 force-pushed the executor-supports-celery branch from df12258 to 17ec0d5 Compare February 19, 2022 18:42
@alexchen8 alexchen8 force-pushed the executor-supports-celery branch from 17ec0d5 to 4e69c85 Compare February 19, 2022 18:53
@alexchen8
Copy link
Contributor Author

My apologies, just noticed that my initial commit was done using company email address. Had to rebase/force push to use personal email.

@potiuk
Copy link
Member

potiuk commented Feb 26, 2022

Anyone else?

@alexchen8
Copy link
Contributor Author

Bumping this PR for review

@alexchen8 alexchen8 closed this Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:Scheduler including HA (high availability) scheduler provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants