Skip to content

[CLI] Various improvements to batch cancel/kill and co. #3943

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[CLI] Various improvements to batch cancel/kill and co. #3943

wants to merge 5 commits into from

Conversation

@slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented Nov 4, 2025

Mitigates #3857

@slinkydeveloper slinkydeveloper changed the title Add a limit to the batch cancel/kill and co. [WIP] Add a limit to the batch cancel/kill and co. Nov 4, 2025
@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented Nov 4, 2025 •
edited
Loading

Tested with 2milion invocaitons, and it does work... Takes a bit, but works ok-ish. Also with the progress bars now, it seems like it's doing something!

@slinkydeveloper slinkydeveloper changed the title [WIP] Add a limit to the batch cancel/kill and co. Add a limit to the batch cancel/kill and co. Nov 4, 2025
@slinkydeveloper slinkydeveloper mentioned this pull request Nov 4, 2025
Open
@slinkydeveloper slinkydeveloper changed the title Add a limit to the batch cancel/kill and co. [CLI] Add a limit to the batch cancel/kill and co. Nov 4, 2025
@slinkydeveloper slinkydeveloper changed the title [CLI] Add a limit to the batch cancel/kill and co. [CLI] Various improvements to batch cancel/kill and co. Nov 5, 2025
muhamadazmy
muhamadazmy requested changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@muhamadazmy muhamadazmy left a comment

Choose a reason for hiding this comment

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

Thanks @slinkydeveloper for this PR and sorry it took too long to review. I left couple of comments below.

cli/src/commands/invocations/cancel.rs Outdated
);
return Ok(());
}
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
Copy link
Contributor

@muhamadazmy muhamadazmy Nov 6, 2025

Choose a reason for hiding this comment

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

Suggested change
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
render_simple_invocation_list(&invocations, ops.limit);

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Nov 6, 2025

Choose a reason for hiding this comment

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

This was intentional, let me tell you my thinking:

  • You want to kill/cancel/whatev maaaany invocations
  • Printing all those IDs has a cost, ain't cheap, and you can see it actually takes time here in this command.
  • Are you, as a user, actually gonna read them all?! Probably not.
  • And even if you want to, when you have such a large output, your terminal emulator will start chopping the output anyway!

So it boils down to: I have this opts.limit here in case you see the query failing/crashing because it's too big, or taking too long, but my operation here was legit in the first place, I actually want to do that big operation killing all Greeter requests. But I don't really care what i'm killing anyway, i know it's gazillion of invocations anyway.

Do you think this makes sense?

Also invocations.len() will be equal to ops.limit anyway, so this change itself won't have effect, right?

cli/src/commands/invocations/pause.rs Outdated
};

render_simple_invocation_list(&invocations);
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
Copy link
Contributor

@muhamadazmy muhamadazmy Nov 6, 2025

Choose a reason for hiding this comment

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

Suggested change
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
render_simple_invocation_list(&invocations, opts.limit);

cli/src/commands/invocations/purge.rs Outdated
};

render_simple_invocation_list(&invocations);
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
Copy link
Contributor

@muhamadazmy muhamadazmy Nov 6, 2025

Choose a reason for hiding this comment

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

Same

cli/src/commands/invocations/restart_as_new.rs Outdated
};

render_simple_invocation_list(&invocations);
render_simple_invocation_list(&invocations, DEFAULT_BATCH_INVOCATIONS_OPERATION_LIMIT);
Copy link
Contributor

@muhamadazmy muhamadazmy Nov 6, 2025

Choose a reason for hiding this comment

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

Here also

@slinkydeveloper
Add a limit to the batch cancel/kill and co. Mitigates restatedev#3857
42f37c1
@slinkydeveloper
Introduce a utility for running concurrent map on a stream, limitin…
bc7f278 
…g the concurrency.
@slinkydeveloper
Assorted improvements to cancel/kill:
4cfe09f 
* Use the limited concurrency map to execute concurrently the admin api commands. Limiting concurrency to 500 seems to be the way to not make it fail 🤷 There's also the mistery of 1011...
* Don't print twice the invocations we're going to kill, there's no need. Just print the failed ones, if any. Same for pause/resume, etc.
* Limit the amount of output too. This is a problem when killing 2milion invocations...
* Add progress bar when reading the invocations
* Add another progress bar when cancelling them
@slinkydeveloper
Whatev CI asked me to do
3226a75
@slinkydeveloper
Feedback
49d7938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@muhamadazmy muhamadazmy muhamadazmy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@slinkydeveloper @muhamadazmy