Skip to content

Comments

Make --executor flag work for breeze start-airflow command#30644

Merged
potiuk merged 26 commits intoapache:mainfrom
utkarsharma2:Breeze-Executor
Jun 8, 2023
Merged

Make --executor flag work for breeze start-airflow command#30644
potiuk merged 26 commits intoapache:mainfrom
utkarsharma2:Breeze-Executor

Conversation

@utkarsharma2
Copy link
Contributor

@utkarsharma2 utkarsharma2 commented Apr 14, 2023

Introduced --executor flag for command breeze start-airflow which accepts CeleryExecutor, and LocalExecutor.
related: #28412

@utkarsharma2 utkarsharma2 changed the title Breeze executor Make --executor flag works for breeze start-airflow command Apr 14, 2023
@utkarsharma2 utkarsharma2 marked this pull request as draft April 14, 2023 14:56
@uranusjr uranusjr changed the title Make --executor flag works for breeze start-airflow command Make --executor flag work for breeze start-airflow command Apr 18, 2023
@uranusjr
Copy link
Member

Would it be a good idea to make it an option on shell as well?

@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Apr 18, 2023

@potiuk, CeleryExecutor, LocalExecutor, CeleryKubernetesExecutor, and KubernetesExecutor support many configuration options in airflow config, should we expose all of them to the user? or can we expose some of them?

Also, what would be a desirable format for exposing config options?
Options:

  1. We can have one --celery that can accept config string in format worker_concurrency=16, flower_host=0.0.0.0 format.
  2. We can expose one parameter for each possible config. Like --celery_worker_concurrency, --celery_flower_host.

@utkarsharma2
Copy link
Contributor Author

@uranusjr Do you think it should be part of this PR or can we create a separate ticket? either way, I'll be happy to pick up the task :)

@uranusjr
Copy link
Member

I’m not sure how involved it would be; if it’s just a handful of lines let’s include it in this PR, otherwise we can split.

@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Apr 18, 2023

I’m not sure how involved it would be; if it’s just a handful of lines let’s include it in this PR, otherwise we can split.

@uranusjr Sure, I'll check what needs to be done to extend this flag to shell

@potiuk
Copy link
Member

potiuk commented Apr 18, 2023

I think all of the executors could be gradually added but we should start small.

BTW.. This is just a beginning and draft of the change. This PR needs far more to be even close to completion.

Currently Breeze does NOT support directly running anythong else than airflow with Local (mysql/postgres/mssql) or Sequential (sqlite) executor. In order to that more things nee to happen:

  1. When we choose Celery Executor, we should force starting the celery integration (which has celery, redis, possibly we should also - optionally - start flower there)

  2. For Kubernetes Executir we need to connect to an external K8S cluster with capabilities of running PODs on them or we should start kind cluster, similarly to what we do in breeze k8s commands and we should be able to talk to the k8s cluster via the API

I think we should start with these two and once we have them working, we could add the "combos" (CeleryKubernetes will require to have both Celery and K8S started)

The idea here is that by running `

@potiuk
Copy link
Member

potiuk commented Apr 18, 2023

Would it be a good idea to make it an option on shell as well?

Yep. We should

@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Apr 19, 2023

I think all of the executors could be gradually added but we should start small.

BTW.. This is just a beginning and draft of the change. This PR needs far more to be even close to completion.

Currently Breeze does NOT support directly running anythong else than airflow with Local (mysql/postgres/mssql) or Sequential (sqlite) executor. In order to that more things nee to happen:

1. When we choose Celery Executor, we should force starting the celery integration (which has celery, redis, possibly we should also - optionally - start flower there)

2. For Kubernetes Executir we need to connect to an external K8S cluster with capabilities of running PODs on them or we should start kind cluster, similarly to what we do in `breeze k8s` commands and we should be able to talk to the k8s cluster via the API

I think we should start with these two and once we have them working, we could add the "combos" (CeleryKubernetes will require to have both Celery and K8S started)

The idea here is that by running `

@potiuk Sure, I'll get started with Celery Executor and Kubernetes Executors for now :)

@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented May 29, 2023

@potiuk Is it okay to have Celery and a Local executor for now? and I'll raise a separate PR for Kubernetes?

If it's okay, this PR is ready to review.

@potiuk
Copy link
Member

potiuk commented May 29, 2023

Yep. It's perfectly fine. K8S executor is much more complex to work on. I will take a look in the next few days (I am less available over the next few days).

@utkarsharma2 utkarsharma2 marked this pull request as ready for review May 29, 2023 09:16
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fantastic thanks!

@potiuk potiuk merged commit aa479b9 into apache:main Jun 8, 2023
@ferruzzi
Copy link
Contributor

ferruzzi commented Jun 9, 2023

That's funny, literally just went looking for this feature today to try breeze start-airflow with the celery executor. 👍 Nice timing :)

@utkarsharma2
Copy link
Contributor Author

@uranusjr I raised a PR for adding the executor option to the shell command here - #31861

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants