Skip to content

[AIRFLOW-1383] Add no_dag_reset option to airflow clear command#2420

Closed
chronitis wants to merge 4 commits intoapache:masterfrom
lifesum:airflow-1383
Closed

[AIRFLOW-1383] Add no_dag_reset option to airflow clear command#2420
chronitis wants to merge 4 commits intoapache:masterfrom
lifesum:airflow-1383

Conversation

@chronitis
Copy link
Copy Markdown
Contributor

@chronitis chronitis commented Jul 6, 2017

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

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

The CLI command airflow clear does not expose any way to set the reset_dag_runs parameter of the underlying DAG.clear() method.

This means that airflow clear will always cause affected DAG runs to be set to state=RUNNING, and immediately try and re-run the cleared tasks. In some circumstances it might not be desirable to immediately re-run everything.

Tests

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

A simple test for the CLI syntax is added to tests/core.py

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
    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"

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 6, 2017

Codecov Report

Merging #2420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2420   +/-   ##
======================================
  Coverage    78.1%   78.1%           
======================================
  Files         201     201           
  Lines       16464   16464           
======================================
  Hits        12859   12859           
  Misses       3605    3605
Impacted Files Coverage Δ
airflow/bin/cli.py 64.59% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e947c6c...9466816. Read the comment docs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than having to do reset_dag_runs=not args.no_dag_reset at the call site, would we do something like "store_false", metavar="reset_dag_runs", default=True)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

metavar was not what I meant.

'reset_dag_runs': Arg(... "store_false", default=True) was what I meant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not possible without further changes; the Arg() namedtuple doesn't include dest, and the generated variable name is not set by the dictionary key; you'd have to do --reset_dag_runs=0 or something similar, which seems unnecessarily ugly.

tests/core.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel that some assertion that the flag does what we expect is needed here please.

(Late review, sorry!)

@chronitis
Copy link
Copy Markdown
Contributor Author

@ashb I've extended the test to assert the state of a created DAG run before and after clearing, which should cover the intended functionality.

I haven't changed the CLI option - as per the comment above, the current airflow/bin/cli.py:CLIFactory does not provide an option to rename the output variable from the CLI flag; wider changes there are probably out-of-scope. The not args.foo idiom is used a number of other times in any case.

One of the kubernetes tests failed for reasons that look unrelated and probably transitory. All the other tests passed.

@stale
Copy link
Copy Markdown

stale bot commented Dec 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 10, 2018
Gordon Ball added 4 commits December 11, 2018 08:48
The underlying DAG.clear() function has a parameter reset_dag_runs
(default True) which controlls whether DAG runs for which a task was
cleared will be set to state=RUNNING. The current CLI doesn't expose
this option; this adds a flag (no_dag_reset) which prevents runs being
set to RUNNING after clearing, for cases where it is desirable to clear
jobs but not immediately have them re-running.
With --no_dag_reset, the state of the DAG should not change to RUNNING
after calling clear.
@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 11, 2018
@ron819
Copy link
Copy Markdown
Contributor

ron819 commented Jan 16, 2019

@ashb did you miss this one?

@feng-tao
Copy link
Copy Markdown
Member

I may be wrong, but I think we have supported something similar in 4c6f1fd

@feng-tao feng-tao closed this Jan 16, 2019
@chronitis chronitis deleted the airflow-1383 branch June 26, 2019 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants