Bad build ids api changes #310

dnr · 2023-08-30T20:04:32Z

What changed?

Add MarkBadBuild option to UpdateWorkerBuildIdCompatibilityRequest and related messages
Add build-id-based option to reset, including RESET_TYPE_BUILD_ID to ResetType in BatchOperationReset

Why?
Help recover from a bad deployment situation

Breaking changes
no

bergundy · 2023-08-30T20:25:39Z

temporal/api/batch/v1/message.proto

+  // For reset_type == RESET_TYPE_BUILD_ID only:
+  // Resets to before the first workflow task processed by the specified build id.
+  string reset_build_id = 4;


Might be worth wrapping in a message in case we want to allow more options here. I'm already thinking that some users may not want to reset past the current run (e.g. when a workflow continues as new).

I made a new message for overall ResetOptions and put that flag (only current run) at the top level, since it could apply to other reset types. So then build_id didn't need a message again. I'm not opposed to a message there but are there really reset options that apply to build ids only?

bergundy · 2023-08-30T20:28:32Z

temporal/api/errordetails/v1/message.proto

+    // If this failure is due to the polling build id being marked as a bad build.
+    bool is_bad_build = 2;


Maybe change to a reason enum to support other reasons in the future?

can you imagine anything else (that would fit generally under NewerBuildExistsFailure instead of some other error type)?

Not at the moment, but we didn't see this one coming either.
As a general guideline, I prefer enums even for dual states, you can't predict how this evolve so early on.

Hmm. Except without another example it's not clear if these should be exclusive (like an enum) or orthogonal (like other fields). E.g. a build can be both not-default and also a bad build.

I'm still not sure about this one. Note that these two fields are not exclusive and actually maybe we wouldn't set default_build_id if the default was marked bad without a replacement. I.e. the options are:

there is a new default build id, but the one you polled with is not marked bad

there is a new default build id, also btw the one you polled with is bad

there is no default since there was one and it got marked bad, so now nobody gets any tasks

At this point it's really the name of the message/error type that's not accurate, but I don't want to change that now

TBH, I don't think we need a custom error here. Just use OutOfRange with an informative error message. We don't do much with this information SDK side anyways.

We already have the custom error type, I'm just adding a field to it

I know, this is more of a lesson for next time.

bergundy · 2023-08-30T20:29:52Z

temporal/api/workflowservice/v1/request_response.proto

+        // The id of a `WORKFLOW_TASK_COMPLETED`,`WORKFLOW_TASK_TIMED_OUT`, `WORKFLOW_TASK_FAILED`, or
+        // `WORKFLOW_TASK_STARTED` event to reset to.
+        int64 workflow_task_finish_event_id = 4;
+        string reset_build_id = 7;


same, consider making this a message instead of a string.

bergundy · 2023-08-30T20:30:34Z

temporal/api/taskqueue/v1/message.proto

+    // Build ids that have been marked as bad. If the last element in build_ids is also in
+    // bad_build_ids, then no tasks will be dispatched on this version set until the default is
+    // changed.
+    repeated string bad_build_ids = 2;


In the internal representation, you can add a new state in the enum, this is fine here though.

yup, that was the plan. we could try to transition the string list to a message but... seems more trouble than it's worth. I don't like the repetition but it's the simplest thing here. I guess it could be a parallel array of bools but that's worse

Yup, this is the best option given the current API, let's keep it and avoid breaking existing clients.

dnr · 2023-08-30T21:22:27Z

temporal/api/batch/v1/message.proto

+  // For reset_type == RESET_TYPE_BUILD_ID only:
+  // Resets to before the first workflow task processed by the specified build id.
+  string reset_build_id = 4;


dnr · 2023-08-30T21:23:33Z

temporal/api/errordetails/v1/message.proto

+    // If this failure is due to the polling build id being marked as a bad build.
+    bool is_bad_build = 2;


can you imagine anything else (that would fit generally under NewerBuildExistsFailure instead of some other error type)?

dnr · 2023-08-30T21:25:58Z

temporal/api/taskqueue/v1/message.proto

+    // Build ids that have been marked as bad. If the last element in build_ids is also in
+    // bad_build_ids, then no tasks will be dispatched on this version set until the default is
+    // changed.
+    repeated string bad_build_ids = 2;


yup, that was the plan. we could try to transition the string list to a message but... seems more trouble than it's worth. I don't like the repetition but it's the simplest thing here. I guess it could be a parallel array of bools but that's worse

dnr · 2023-08-30T21:26:50Z

temporal/api/batch/v1/message.proto

+
+  // For reset_type == RESET_TYPE_BUILD_ID only:


I wish this was a oneof in the first place :(

I just went and made it a oneof. I think it's the cleanest way to deal with the new reset options.

yiminc · 2023-09-01T17:19:35Z

temporal/api/workflowservice/v1/request_response.proto

+        // `WORKFLOW_TASK_STARTED` event to reset to.
+        int64 workflow_task_finish_event_id = 4;
+        // Reset to before a specific build id.
+        temporal.api.common.v1.BuildIdResetOptions reset_build_id = 7;


Currently, we have reset option to reset to first or last workflow task. And the caller (batch workflow) has to figure out which workflow task event_id that option maps to. That is different from this new option which history service has to figure out which event_id it maps to.
I like this new approach, but it make it inconsistent with existing ones. Probably should (in future) add the first / last workflow task as option here and move those logic from batch workflow to history service.

Just realize this is already requested as temporalio/temporal#1338

I see. Do you think we should extract all this into a whole new message and migrate to that here? Otherwise we have a oneof here, and then an enum for the batch request forever... instead we could make a new message and use it for both, and deprecate the old fields.

Or we could have the batch workflow look up the reset point, but that seems silly since history owns it.

I just made a new message and used it in both places. It'll require some compatibility code to translate but it shouldn't be too bad.

…quest

dnr · 2023-09-27T17:58:46Z

I changed this to use the new ResetOptions only for the batch request, not the individual request. Using it for the individual request could come later, although maybe there are good reasons to have them all go through batch (so that we do the digging through history on the worker instead of frontend or history).

I also added a build id to ResetPointInfo so it has both a build id and binary checksum now, and we can remove binary checksum later.

There are a few situations that may require some more changes here, but this should be good to go for phase 1.

Sushisource

Structurally makes sense to me. Some stuff to fix in the docstrings.

Sushisource · 2023-09-29T00:12:51Z

temporal/api/batch/v1/message.proto

+
+  // New API (exclusive with `reset_type` and `reset_reapply_type`):


IMO this comment should just say what this is for, and the comments for the older things can call out that they are old/deprecated.

Maybe keep the exclusivity mention, but don't need to say it's "New" I think.

Sushisource · 2023-09-29T00:15:25Z

temporal/api/common/v1/message.proto

+message ResetOptions {
+    // Which workflow task to reset to.
+    oneof target {
+        // Resets to event of the first workflow task completed, or if it does not exist, the event after task scheduled.


What if there's no event after task scheduled either?

I think this wording is slightly more clear:

Suggested change

// Resets to event of the first workflow task completed, or if it does not exist, the event after task scheduled.

// Resets to the first `WORKFLOW_TASK_COMPLETED` event, or if it does not exist, the first `WORKFLOW_TASK_SCHEDULED` event.

I'll just say "first workflow task completed or started event", I think that's enough for this purpose.

Sushisource · 2023-09-29T00:15:45Z

temporal/api/common/v1/message.proto

+    oneof target {
+        // Resets to event of the first workflow task completed, or if it does not exist, the event after task scheduled.
+        google.protobuf.Empty first_workflow_task = 1;
+        // Resets to event of the last workflow task completed, or if it does not exist, the event after task scheduled.


Same comment as w/ first

Sushisource · 2023-09-29T00:16:47Z

temporal/api/common/v1/message.proto

+        // 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;
+        // Resets to before the first workflow task processed by this build id.


Which event, specifically, before the first WFT processed by the build ID?

As I understand it, it doesn't matter whether it's the scheduled, started, or completed/failed/timedout event, the effect is the same for all. I'll just say "first workflow task processed by this build id"

Sushisource · 2023-09-29T00:20:15Z

temporal/api/workflowservice/v1/request_response.proto

+        // If promote_build_id is set, then also promote it to be the default of the set.
+        // If it's not set, then no tasks will be dispatched on the task queue set until another
+        // build id is made the default by another call to UpdateWorkerBuildIdCompatibility that
+        // sets/adds a new default for the set.


This comment implies that bad_build_id is the current default for it's set, but, that might not be the case. Presumably you really only care to do this if it is, but, it's worth mentioning that.

Sushisource · 2023-09-29T00:21:06Z

temporal/api/workflowservice/v1/request_response.proto

+        // Mark a build as "bad" for a particular task queue. This means that:
+        // 1. the build will not receive any more tasks (workflow or activity)
+        // 2. it may not be promoted to the set default anymore
+        // See the MarkBadBuild for more information.


Suggested change

// See the MarkBadBuild for more information.

// See MarkBadBuild for more information.

dnr

Splitting this into a separate PR for reset, and the rest can come later

dnr · 2023-11-28T03:03:04Z

temporal/api/common/v1/message.proto

+message ResetOptions {
+    // Which workflow task to reset to.
+    oneof target {
+        // Resets to event of the first workflow task completed, or if it does not exist, the event after task scheduled.


I'll just say "first workflow task completed or started event", I think that's enough for this purpose.

dnr · 2023-11-28T03:04:58Z

temporal/api/common/v1/message.proto

+        // 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;
+        // Resets to before the first workflow task processed by this build id.


As I understand it, it doesn't matter whether it's the scheduled, started, or completed/failed/timedout event, the effect is the same for all. I'll just say "first workflow task processed by this build id"

dnr · 2023-11-28T03:14:56Z

See #333

bergundy reviewed Aug 30, 2023

View reviewed changes

dnr commented Aug 30, 2023

View reviewed changes

yiminc reviewed Sep 1, 2023

View reviewed changes

dnr added 7 commits September 27, 2023 18:25

add MarkBadBuild option

776bbb6

add reset to build id options

a9fcba2

BuildIdResetOptions

e9efa10

add common ResetOptions

b3c095e

update comments

88e9fb3

use ResetOptions only for batch for now, not ResetWorkflowExecutionRe…

0346b32

…quest

add build id to ResetPointInfo

e4fcfd3

dnr force-pushed the bb1 branch from 1082962 to e4fcfd3 Compare September 27, 2023 17:55

dnr marked this pull request as ready for review September 27, 2023 17:58

dnr requested review from a team as code owners September 27, 2023 17:58

Sushisource reviewed Sep 29, 2023

View reviewed changes

dnr commented Nov 28, 2023

View reviewed changes

dnr closed this Nov 28, 2023

		// If this failure is due to the polling build id being marked as a bad build.
		bool is_bad_build = 2;


		// New API (exclusive with `reset_type` and `reset_reapply_type`):

	// Resets to event of the first workflow task completed, or if it does not exist, the event after task scheduled.
	// Resets to the first `WORKFLOW_TASK_COMPLETED` event, or if it does not exist, the first `WORKFLOW_TASK_SCHEDULED` event.

	// See the MarkBadBuild for more information.
	// See MarkBadBuild for more information.

Bad build ids api changes #310

Bad build ids api changes #310

Conversation

dnr commented Aug 30, 2023

bergundy Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr commented Sep 27, 2023

Sushisource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr commented Nov 28, 2023

bergundy Aug 30, 2023 •

edited

Loading