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

Combine workflow reset and workflow reset-batch? #499

Closed
josh-berry opened this issue Mar 15, 2024 · 11 comments
Closed

Combine workflow reset and workflow reset-batch? #499

josh-berry opened this issue Mar 15, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@josh-berry
Copy link
Collaborator

We should explore whether we want to combine the workflow reset and workflow reset-batch commands. AFAIK, they do the same thing, and the only difference is whether they operate on a single workflow or multiple workflows.

There are other workflow commands, like workflow terminate and workflow cancel, that also support operating on multiple workflows at once. So having reset-batch be separate from reset seems inconsistent.

@josh-berry josh-berry added the enhancement New feature or request label Mar 15, 2024
@josh-berry josh-berry changed the title [Feature Request] Combine workflow reset and workflow reset-batch Combine workflow reset and workflow reset-batch? Mar 15, 2024
@bergundy
Copy link
Member

reset-batch uses client side batches. We want to deprecate it and use the server's batch reset capabilities.
When we do that it'll be a completely different API and all of these options will be gone:

   --exclude-file value                    Input file that specifies Workflow Executions to exclude from a reset.
   --input-file value                      Input file that specifies Workflow Executions to reset. Each line contains one Workflow Id as the base Run and, optionally, a Run Id.
   --input-parallelism value               Number of goroutines to run in parallel. Each goroutine processes one line per second. (default: 1)
   --input-separator                         Separator for the input file. The default is a tab (  ). (default: "\t")

We should not port this command as is in the rewrite.

@cretz
Copy link
Member

cretz commented Mar 18, 2024

The question is whether this will be enough of a functionality loss moving from client side to server side (I haven't compared the server capabilities compared to the CLI behavior today). But if it's not considered too much of a functionality loss, I definitely agree, client-side batch is bad.

@dnr
Copy link
Member

dnr commented Mar 18, 2024

We decided a while ago that dropping client-side batch is fine. We can add more features to server-side batch as requested (and tctl still exists in the meantime).

@josh-berry
Copy link
Collaborator Author

Sounds good—though I thought reset-batch also supports more --query functionality than batch does today, does it not? Do we have something to track that missing functionality already?

@josh-berry
Copy link
Collaborator Author

D'oh, so I'm realizing we actually handled batch resets in #473. So I think this can be closed so long as we understand (and have other issues for) whatever functionality is still missing server-side and CLI-side.

@cretz
Copy link
Member

cretz commented May 30, 2024

Here are the features I am seeing that were in manual client-side batch reset in tctl/0.11.0 that are not in server-side batch reset in 0.12.0:

  • --input-file/--exclude-file - Ability to provide/exclude specific workflow/run IDs. I think this can be done on the query.
  • --input-parallelism - Ability to set client-side concurrency. Server-side batch handles its own parallelism.
  • --skip-current-open - Ability to skip if there are any open runs for the workflow ID. This can be done on the query.
  • --skip-base-is-not-current - Ability to only reset if the run ID is the current for the workflow ID. I don't see how to limit query to only latest run ID for a workflow ID. Is this what https://github.com/temporalio/api/blob/8c57e7b694efd2fee3180c9422d40d421bf14df4/temporal/api/common/v1/message.proto#L179 is? We don't expose it in CLI today. Regardless, this never could be 100% accurate client side because a workflow could roll over mid-batch.
  • --non-deterministic - Ability to only reset ones with non-deterministic as the last task failure. This is not supported on server-side batch reset.
  • --type LastContinuedAsNew - Ability to reset to the continue as new task on the previous run ID (the implementation is shoddy though). This is not supported on server-side batch reset.
  • --dry-run - Ability to do everything but the actual reset. This is not supported on server-side batch reset.

Will confer with team on whether these are needed.

@paulnpdev
Copy link
Member

Ability to only reset if the run ID is the current for the workflow ID. I don't see how to limit query to only latest run ID of current execution chain. Is this what https://github.com/temporalio/api/blob/8c57e7b694efd2fee3180c9422d40d421bf14df4/temporal/api/common/v1/message.proto#L179 is? We don't expose it in CLI today. Regardless, this never could be 100% accurate client side because a workflow could roll over mid-batch.

what do you mean by current execution chain?

@cretz
Copy link
Member

cretz commented May 30, 2024

what do you mean by current execution chain?

I shouldn't have said "of current execution chain", I should have said "for a workflow ID". I have updated the text.

@paulnpdev
Copy link
Member

paulnpdev commented May 30, 2024

Got it.
I see no reason we should not be able to apply the reset to the current run of each workflow key provided by the query, input, or file with optional limitation to only open or closed workflows

@cretz
Copy link
Member

cretz commented May 30, 2024

Agreed, but this is about documenting what does exist vs what could exist. For anything that doesn't exist but we want it to, I support adding to the server. The limitation to only open or only closed can already be done via query filtering IIUC.

@cretz
Copy link
Member

cretz commented May 30, 2024

Closing issue. The commands have already been combined and the differences are documented at #499 (comment).

@cretz cretz closed this as completed May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants