Page MenuHomePhabricator

Disable terminate button if run cannot be cancelled

Authored by alangenfeld on Nov 4 2019, 11:43 AM.


Group Reviewers
Restricted Project
R1:902e1b99a29d: Disable terminate button if run cannot be cancelled

The cancel_pipeline_execution mutation returns a CancelPipelineExecutionFailure if run.status is not PipelineRunStatus.STARTED.

This diff updates the UI to disable the "Terminate" button if run.status !== PipelineRunStatus.STARTED

Test Plan

Execute pipeline, wait for execution to finish/error, verify Terminate button is disabled

Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Nov 4 2019, 11:43 AM
sashank added a reviewer: Restricted Project.Nov 4 2019, 11:44 AM
sashank edited the summary of this revision. (Show Details)
sashank edited the test plan for this revision. (Show Details)
sashank edited the test plan for this revision. (Show Details)
alangenfeld requested changes to this revision.Nov 4 2019, 3:50 PM
alangenfeld added a subscriber: alangenfeld.

in what case is canCancel returning True but the run can not actually be canceled?

84 ↗(On Diff #6175)

thats what this is for

259 ↗(On Diff #6175)


This revision now requires changes to proceed.Nov 4 2019, 3:50 PM

If I had to guess the problem - its that we are not getting false for canCancel even though the run is finished in the last subscription payload due to latency between when the run is finished and when the subprocess execution manager has cleaned up the subprocess

alangenfeld commandeered this revision.Nov 4 2019, 9:09 PM
alangenfeld edited reviewers, added: sashank; removed: alangenfeld.

let me right my wrongs

alangenfeld updated this revision to Diff 6189.Nov 4 2019, 9:09 PM

update based on log stream

sashank added inline comments.Nov 4 2019, 9:15 PM

Should this be:

if (status === PipelineRunStatus.STARTED) { local.canCancel = true }

or we change the cancel_pipeline_execution error to be:

if run.status == PipelineRunStatus.FAILURE or run.status == PipelineRunStatus.SUCCESS:
        return graphene_info.schema.type_named('CancelPipelineExecutionFailure')()

so that the two match up?

sashank accepted this revision.Nov 4 2019, 9:16 PM
This revision is now accepted and ready to land.Nov 4 2019, 9:16 PM
alangenfeld added inline comments.Nov 4 2019, 9:19 PM

the only authority on setting this true is the execution manager since we may be looking at a run from a different machine - but we know that if a pipeline has reached a terminal state canCancel should certainly be false

ideally we would just be getting up to date status and canCancel fields from each subscription update but thats very tricky for both perf reasons and due to how the python server subscriptions work

sashank added inline comments.Nov 4 2019, 9:31 PM

That makes sense, thanks for clarifying!

This revision was landed with ongoing or failed builds.Nov 4 2019, 9:52 PM
This revision was automatically updated to reflect the committed changes.