Add build-id-based reset #333
Conversation
temporal/api/batch/v1/message.proto
Outdated
| // Deprecated (use `options`): | ||
| // Reset type. | ||
| temporal.api.enums.v1.ResetType reset_type = 1; | ||
| // History event reapply options. |
There was a problem hiding this comment.
I assume this is also deprecated?
There was a problem hiding this comment.
Yeah, it's included in options. I guess I should comment each individually
temporal/api/batch/v1/message.proto
Outdated
| // Describes what to reset to and how (exclusive with `reset_type` and `reset_reapply_type`) | ||
| temporal.api.common.v1.ResetOptions options = 4; | ||
|
|
||
| // Reset type (deprecated, use `options`). | ||
| temporal.api.enums.v1.ResetType reset_type = 1; | ||
| // History event reapply options (deprecated, use `options`). | ||
| temporal.api.enums.v1.ResetReapplyType reset_reapply_type = 2; |
There was a problem hiding this comment.
Why not use a oneof if these are exclusive?
There was a problem hiding this comment.
I don't think oneof helps here. I'm moving those fields into options and deprecating them. Callers should just migrate to options, but I don't want to break existing code yet.
Maybe I should write "if this is set, the other fields are ignored" on options instead?
There was a problem hiding this comment.
I just totally spaced that they said deprecated.
I do like that change in docstring though 👍
| // The id of a specific `WORKFLOW_TASK_COMPLETED`,`WORKFLOW_TASK_TIMED_OUT`, `WORKFLOW_TASK_FAILED`, or | ||
| // `WORKFLOW_TASK_STARTED` event to reset to. |
There was a problem hiding this comment.
I always got confused by this. Why not simply reset to the workflow task scheduled event?
There was a problem hiding this comment.
As far as I understand it, it always does effectively do that. It just branches history at the workflow task started event and adds a workflow task failed, so nothing from that workflow task takes effect. It's just that you can specify any of those types since they are always contiguous in history.
It does seem like it would be cleaner to branch before the workflow task scheduled event and then create a new one, but I guess there was/is probably a good reason to do it this way. I'm not proposing to change this here, I just copied the comment from ResetWorkflowExecutionRequest
| google.protobuf.Empty last_workflow_task = 2; | ||
| // The id of a specific `WORKFLOW_TASK_COMPLETED`,`WORKFLOW_TASK_TIMED_OUT`, `WORKFLOW_TASK_FAILED`, or | ||
| // `WORKFLOW_TASK_STARTED` event to reset to. | ||
| int64 workflow_task_id = 3; |
There was a problem hiding this comment.
How are user suppose to use this in batch reset case?
There was a problem hiding this comment.
They're not, this is so we can use the same message for single-workflow reset in the future. I'll add a comment
|
|
||
| // If true, limit the reset to only within the current run. (Applies to build_id targets and | ||
| // possibly others in the future.) | ||
| bool current_run_only = 11; |
There was a problem hiding this comment.
I'm super late to this review but if this doesn't apply to all targets, maybe it would have been better to make build_id a message and put this in the message.
This would also make build_id extensible in the future.
message BuildId {
string build_id = 1;
bool current_run_only = 2;
}
...
BuildId build_id = 4;
What changed?
ResetOptionsto encapsulate options for resetting one or more workflowstargetforResetOptionsuses aoneofto allow a reset type to have an argument.BatchOperationResetusesResetOptionsResetPointInfocontains abuild_idin addition tobinary_checksum(which is now marked deprecated).Why?
Help recover from a bad deployment situation
Breaking changes
no, the old reset options will continue to be supported