[AIRFLOW-184] Add mark success to CLI

[jgao54]

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

• (replace with a link to AIRFLOW-184)

Reminders for contributors:

• Your PR's title must reference an issue on Airflow's JIRA. For example, a PR called "[AIRFLOW-1] My Amazing PR" would close JIRA issue Improving the search functionality in the graph view #1. Please open a new issue if required!
• You must add an Apache License header to all new files.
• Please squash your commits when possible and follow the 7 rules of good Git commits.

[criccomini]

Looks like tests are unhappy:

TypeError: mark_success() got an unexpected keyword argument 'confirm_propt'

Typo? :)

Also saw this error, which looks a little more serious:

assert "Wait a minute" in response.data.decode('utf-8')
AssertionError

[criccomini]

Typo? prompt

[bolkedebruin]

I think the overall approach needs to be updated. Ie. you do not mark a "dag" as a success but your mark a "DagRun" and/org its underlying tasksinstances as a success. As such I dont like "dag.mark_success".

Creating TIs outside a dagrun is becoming a big no-no and I would say it is even worse to do so in a "view" component, besides it is indeed not required anymore since #1506 as tasks are eagerly created.

All in all, I really like the functionality but please make it DagRun based.

[criccomini]

Agree with @bolkedebruin's comments about DAG run vs dag.

[criccomini]

Nit: this is backwards incompatible, and could lead to issues if someone was using unnamed params and providing a session. Worth a quick glance, but I'm OK with this change.

[criccomini]

Actually, the only place that I see this used is in jobs.py (twice), and in tests. Would prefer removing the to_list param, and just returning tis. Then updating code that makes the call to use .all() on the return object.

Ack that this is backwards compatible, but seems very limited usage.

[criccomini]

Took another pass and left some comments.

Make sure to take a look at the failing test. Looks like a legit failure after your changes.

Also, we should block on @bolkedebruin for this one, I think, since it's touching DagRun, and he's refactoring stuff there.

[artwr]

Just curious to see if you still intend to refactor this, because I think there is a definite use case for this to be enabled. But maybe this can wait for a REST API.

[jgao54]

@artwr @criccomini @bolkedebruin yeah, definitely still intend to refactor this. The current mark success in the UI has a few bugs, (using mark_success in conjunction with past, upstream, or downstream flag throws the error page in some cases). This pr will address those issues as well. I'm ok with waiting for REST API if it's coming up soon, otherwise I think it may be more beneficial to have this merged (once I clean it up =P ).

[r39132]

I think a mark success feature is a good one. I have been looking for one and don't like running a backfill to achieve it. There is one issue I noticed on the current 1.7.1.3 release.

I was 3 days behind on an hourly run. I was hoping a single backfill -m would suffice to mark everything success till current. However, since DAG Runs/Task Instances are generated in little groups by scheduler cycles, that was not the case.... I had to continuously run mark success.. for an hour.

[criccomini]

I'm ok with waiting for REST API if it's coming up soon, otherwise I think it may be more beneficial to have this merged (once I clean it up =P ).

I think it's worth getting this PR done. The REST API is likely a long ways away.

[jgao54]

@bolkedebruin I noticed that subdag's TIs are created independently rather than via DagRun (there are currently no dagrun being created for subdags). Was wondering if this is intentional? imho it would make sense to create dagruns for subdags as well if we want to support mark success / clear a subdag via dagrun.

[bolkedebruin]

@jgao54 that is indeed so, but old style. I have a PR outstanding that does add dag runs to backfills (that is how subdags run) but it needs reviewing/testing

[jgao54]

@bolkedebruin ah I see it! In that case I'll hold off this PR until yours is merged. I will need to make a bit of changes accordingly, but it will be cleaner.

[bolkedebruin]

If you can please review the other pr and +1 it. Thanks!

[criccomini]

@bolkedebruin what's the PR?

[jgao54]

@criccomini #1667

[jgao54]

@criccomini @bolkedebruin @jlowin Updated this PR to reflect the changes from #1667

[criccomini]

cc @aoen

[criccomini]

Seems like all tests are failing?

[jgao54]

@criccomini thanks! fixed

[codecov-io]

Current coverage is 64.75% (diff: 79.51%)

Merging #1590 into master will increase coverage by 0.01%

@@             master      #1590   diff @@
==========================================
  Files           127        127          
  Lines          9498       9528    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6149       6170    +21   
- Misses         3349       3358     +9   
  Partials          0          0          

Powered by Codecov. Last update 821bdb5...7c5199e

[r39132]

@jgao54 Resurrecting this.. please rebase and prepare for review or request to close.

[r39132]

@jgao54 Do you still want to merge this? If not, I will close for inactivity.

[r39132]

@jgao54 closing for inactivity. Please reopen when ready to submit.