Skip to content

[AIRFLOW-6310] Group CLI commands in subcommands#6858

Closed
mik-laj wants to merge 3 commits intoapache:masterfrom
PolideaInternal:AIRFLOW-6310
Closed

[AIRFLOW-6310] Group CLI commands in subcommands#6858
mik-laj wants to merge 3 commits intoapache:masterfrom
PolideaInternal:AIRFLOW-6310

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Dec 19, 2019

This is not finished PR. It is necessary to update the tests, but I decided to share it to get an opinion before investing more time. I would like to suggest that you group several commands to help users find the right command.

Old command New command
airflow kerberos airflow run kerberos
airflow webserver airflow run webserver
airflow scheduler airflow run scheduler
airflow worker airflow run worker
airflow flower airflow run flower
airflow config airflow config show
airflow rotate_fernet_key airflow config rotate_fernet_key

After execution we will have only two clean commands in airflow (without subcomands):

  • sync_perm
  • version

Another suggestion is to use airflow commands instead of airflow run, but it will be much longer.

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-XXX
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@turbaszek
Copy link
Member

I like the overall idea but what if we would like to add stop command to celery worker? Then it would be more sensible imho to do airflow worker stop than airflow stop worker.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 19, 2019

If we add a command then we will wonder how we will do it. For now, it does not promise that any such command would arise. Services are managed by specialized tools, e.g. systemd. Example configuration for systemd is available here: https://github.com/apache/airflow/tree/9dce1f0740f69af0ee86709a1a34a002b245aa3e/scripts/systemd

@turbaszek
Copy link
Member

I agree but at least lets try to be consistent, now we have:
airflow run something - airflow verb noun
airflow config do_something - airflow noun verb
in my opinion it's confusing especially after the long discussion we had when whole CLI was refactored.

@kaxil
Copy link
Member

kaxil commented Dec 19, 2019

Old command New command
airflow kerberos airflow run kerberos
airflow webserver airflow run webserver
airflow scheduler airflow run scheduler
airflow worker airflow run worker
airflow flower airflow run flower

I think the above change is not needed and I am "-1" if we change it. airflow run --raw is already used at many places. Also logically they are separate commands and we don't need refactor for it.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 19, 2019

What do you think about components? I think that some way of grouping these commands would be helpful for users, but I'm not sure about the solution.
image

@kaxil
Copy link
Member

kaxil commented Dec 19, 2019

The current CLI is a lot better (https://airflow.readthedocs.io/en/latest/cli-ref.html) than the ones in 1.10.x version https://airflow.readthedocs.io/en/1.10.6/cli.html

We have grouped the commands and I think the current grouping is all that we need.

@ashb
Copy link
Member

ashb commented Dec 20, 2019

Yeah I'm -0.5 on this too, as they are separate in what task each component permforms, and I "run x" isn't a pattern that I've seen with other CLIs

@potiuk
Copy link
Member

potiuk commented Dec 20, 2019

Agree with @ashb and @kaxil on that one (-0.75 for me ;) ). It's ok to have those "run" type commands as top level. It's more intuitive even is command tree has leaves at different levels (if you see what I mean).

@mik-laj mik-laj closed this Dec 21, 2019
@turbaszek
Copy link
Member

What do you think about grouping celery commands under airflow celery [CMD]? Main reason for that is that the name worker in future can be used in wider context.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 22, 2019

Good idea. I will create separate PR to start discussion.

@ashb
Copy link
Member

ashb commented Dec 22, 2019

For the Knative executor we were thinking of having airflow worker start up either Celery worker or a Knative worker based on the executor config option.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 23, 2019

I created PR about celery command:
#6877

@potiuk
Copy link
Member

potiuk commented Dec 23, 2019

I really like the "celery" , "native" hiding when particular executor is not configured more than having reused "worker" command. Especially that their use might be different.

BTW. @ashb, @dimberman -> do you really want to follow the KNative route? I honestly do not see any benefits over the "native executor" proposal by @nuclearpinguin #6750. KNative one implements subset of the functionality of the "native executor" by adding many more components that are not really needed to get what we want (http server, knative, istio) without real benefit and with limitations introduced by KNative. The "native executor" has a chance to be the fastest, most reliable, easily scalable and simplest executor we can think about. I have not seen yet a single problem/limitation with this design.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants