Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add experiment cancel and kill endpoints [DET-3581] #1026

Merged

Conversation

hamidzr
Copy link
Member

@hamidzr hamidzr commented Aug 5, 2020

Description

Test Plan

Commentary (optional)

@hamidzr hamidzr requested a review from stoksc August 5, 2020 22:08
@cla-bot cla-bot bot added the cla-signed label Aug 5, 2020
@hamidzr hamidzr changed the title Add experiment cancel and kill endpoints [DET-3581] feat: experiment cancel and kill endpoints [DET-3581] Aug 5, 2020
@hamidzr hamidzr changed the title feat: experiment cancel and kill endpoints [DET-3581] feat: add experiment cancel and kill endpoints [DET-3581] Aug 5, 2020
Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

👍 looks good

master/internal/api_experiment.go Outdated Show resolved Hide resolved
addr := actor.Addr("experiments", req.Id).String()
err = a.actorRequest(addr, req, &resp)
if status.Code(err) == codes.NotFound {
return &apiv1.CancelExperimentResponse{}, nil
Copy link
Contributor

@stoksc stoksc Aug 6, 2020

Choose a reason for hiding this comment

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

question: this is ignored to be in line wanting most endpoints to be Idempotent?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct because at this point if actor doesn't find it it means that this experiment exists (in DB) but is in a terminal state. how does that sound to you

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that sounds perfect to me, easiest behavior for a user to design stuff with imo.

@stoksc stoksc assigned hamidzr and unassigned stoksc Aug 6, 2020
Co-authored-by: Bradley Laney <[email protected]>
@hamidzr hamidzr assigned stoksc and unassigned hamidzr Aug 6, 2020
Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

👍

@stoksc stoksc assigned hamidzr and unassigned stoksc Aug 6, 2020
@hamidzr hamidzr merged commit 398cf66 into determined-ai:master Aug 6, 2020
@hamidzr hamidzr deleted the 3581-add-cancel-kill-endpoints branch August 6, 2020 21:54
@dannysauer dannysauer added this to the 0.13.0 milestone Feb 6, 2024
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.

3 participants