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

Add cancel/kill all invocations to a service or service instance #1529

Merged
merged 2 commits into from
May 22, 2024

Conversation

slinkydeveloper
Copy link
Contributor

Fix #827

@slinkydeveloper
Copy link
Contributor Author

!11344 ➜ cargo run -- service cancel Counter --kill                                                                                     ~/projects/work/restate (issues/827) ✗
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s
     Running `target/debug/restate service cancel Counter --kill`
 ID                                      TARGET                    STATUS             
 inv_1cw1Hbl2xwPi1Bapu1dHgUZKXdWstr3uzn  Counter/my-counter-3/add  running     
 inv_16ZFO31Wksqh7sz5A6BE1AHhp7uBCTqG53  Counter/my-counter-2/add  running     
 inv_1l8n0Bsg2WP35PIjesZk9jpqDMDJApOuCl  Counter/my-counter/get    backing-off 

✔ Are you sure you want to kill these invocations? · yes

✅ Request was sent successfully

Comment on lines 73 to 76
for inv in invocations {
let result = client.cancel_invocation(&inv.id, opts.kill).await?;
let _ = result.success_or_error()?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A) Should we put an upper bound on how many we display on the screen? (maybe a few examples and a message that says how many more?)
B) Do you think it'd cause problems to have this be an unbounded loop of cancellations (in situations with potentially large number of invocations)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we put an upper bound on how many we display on the screen?

We could introduce an upper bound, but then this means either that we don't show all the invocations we kill, or we need to shave off some invocations, meaning that during dev the user might have to run many times the kill command to kill all the invocations...

Do you think it'd cause problems to have this be an unbounded loop of cancellations (in situations with potentially large number of invocations)?

Only way to find out is testing it 😄 In principle here we just append commands to the log, so many commands probably will slow down the system a bit while all of those are processed.

@AhmedSoliman
Copy link
Contributor

My 2c:

  • I don't think we should have two cancel commands, cancelling a "service" doesn't read right to me. Alternatively. You might want to merge this logic into inv cancel?
  • Should we make it more explicit that the intention is to cancel all invocations by adding --all?

@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented May 21, 2024

I don't think we should have two cancel commands, cancelling a "service" doesn't read right to me. Alternatively. You might want to merge this logic into inv cancel?

Sounds good to me, i can disambiguate the "query" by checking if the string starts with inv_ or not.

Should we make it more explicit that the intention is to cancel all invocations by adding --all?

I don't see the need for it TBH, it's quite explicit already and asks for confirmation too.

Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

LGTM. Let's run this as an experiment (maybe make it clear that this is a [BETA] command on help?) to see how it behaves and performs before we commit to this interface/behaviour.

@slinkydeveloper
Copy link
Contributor Author

I would like to avoid adding BETA, as we're trying as much as we can to show that restate is stable.

@slinkydeveloper slinkydeveloper merged commit 58479ca into restatedev:main May 22, 2024
4 checks passed
@slinkydeveloper slinkydeveloper deleted the issues/827 branch May 22, 2024 08:40
@AhmedSoliman
Copy link
Contributor

I would like to avoid adding BETA, as we're trying as much as we can to show that restate is stable.

A beta CLI command doesn't mean that restate is not stable! it's a signal that the command interface is experimental and we are likely to change the behavior.

@slinkydeveloper
Copy link
Contributor Author

Yes but it can give the wrong impression that the cancel/kill feature is not stable, and not just the command interface. I also think that the whole CLI is probably experimental and we'll do some changes to it over time, perhaps breaking old commands too...

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.

Kill/cancel invocation range/group
2 participants