Update Workflow/Activity priority - #8396
Conversation
36fa1b9 to
4646a2d
Compare
3b6b5ac to
1e9ae2f
Compare
| errorMessage := fmt.Sprintf( | ||
| "Activity task with this stamp not found. Id: %s,: type: %s, current stamp: %d", | ||
| ai.ActivityId, ai.ActivityType.Name, ai.Stamp) | ||
| return nil, rejectCodeUndefined, serviceerror.NewNotFound(errorMessage) | ||
| return nil, rejectCodeUndefined, serviceerrors.NewObsoleteMatchingTask(errorMessage) |
There was a problem hiding this comment.
This seems much more consistent and a better error type, too.
e7581c5 to
1c4b8b8
Compare
5578ed9 to
69d768f
Compare
552b4d7 to
288a3c2
Compare
| if _, ok := updateFields["retryPolicy"]; ok { | ||
| mergeInto.RetryPolicy = mergeFrom.RetryPolicy | ||
| } |
There was a problem hiding this comment.
This seems like a reasonable addition to me.
There was a problem hiding this comment.
I guess so. I was originally a little confused at the semantics, but it seems reasonable, to allow updating a whole sub-object without listing all the fields
| s.NotNil(descResp.GetPendingActivities()) | ||
| s.Len(descResp.GetPendingActivities(), 1) | ||
| s.ProtoEqual(updatedPriority, descResp.GetPendingActivities()[0].GetPriority()) | ||
| s.ProtoEqual(updatedPriority, descResp.GetPendingActivities()[0].GetActivityOptions().GetPriority()) |
There was a problem hiding this comment.
Other activity options appear to always match the activity's fields 1:1.
| @@ -148,6 +148,7 @@ func processActivityOptionsUpdate( | |||
| ScheduleToStartTimeout: ai.ScheduleToStartTimeout, | |||
| StartToCloseTimeout: ai.StartToCloseTimeout, | |||
| HeartbeatTimeout: ai.HeartbeatTimeout, | |||
| Priority: ai.Priority, | |||
There was a problem hiding this comment.
hmm, this is mergeInto, so mergeActivityOptions will write directly into ai.Priority?
shouldn't we expand it like RetryPolicy? or use common.CloneProto (on both)?
There was a problem hiding this comment.
good call 👍 (there is no ai.RetryPolicy, though)
| if _, ok := updateFields["retryPolicy"]; ok { | ||
| mergeInto.RetryPolicy = mergeFrom.RetryPolicy | ||
| } |
There was a problem hiding this comment.
I guess so. I was originally a little confused at the semantics, but it seems reasonable, to allow updating a whole sub-object without listing all the fields
| if prio, ok := proto.Clone(priority).(*commonpb.Priority); ok { | ||
| opts.Priority = prio | ||
| } |
There was a problem hiding this comment.
| if prio, ok := proto.Clone(priority).(*commonpb.Priority); ok { | |
| opts.Priority = prio | |
| } | |
| opts.Priority = common.CloneProto(priority) |
There was a problem hiding this comment.
(and change the one on line 130 also)
| @@ -155,5 +188,10 @@ func mergeWorkflowExecutionOptions( | |||
| } | |||
| mergeInto.VersioningOverride = mergeFrom.GetVersioningOverride() | |||
| } | |||
|
|
|||
| if _, ok := updateFields["versioningOverride"]; ok { | |||
There was a problem hiding this comment.
move this one before the ones that do sub-fields so they're consistent? (even though it doesn't matter in this case)
| } | ||
|
|
||
| if _, ok := updateFields["priority.priorityKey"]; ok { | ||
| mergeInto.Priority = cmp.Or(mergeInto.Priority, &commonpb.Priority{}) |
There was a problem hiding this comment.
hmm, maybe this should do an if like the updateactivityoptions one to avoid the extra allocation? (I'm not sure it does an extra allocation but it looks like it)
if you want a generic helper to initialize a pointer if it's nil, I'd be okay with that. I've wanted it in various places but it feels a little un-go-like so I've just done if statements or local helpers
There was a problem hiding this comment.
let's go with a simple if 👍
| Stamp: task.event.Data.GetStamp(), | ||
| Priority: task.event.Data.GetPriority(), |
There was a problem hiding this comment.
keep them in the same order as the declaration?
| } | ||
| } | ||
| s.Equal(1+obsoleteWorkflowTaskCount, obsoleteActivityTaskCount, "Expected 1 activity task to be obsolete") | ||
| } |
There was a problem hiding this comment.
nice test, clear and thorough. the metrics stuff doesn't add any flakiness, I hope?
There was a problem hiding this comment.
I don't expect any; it's all in-memory and only checked in latency insensitive places AFAICT
0991911 to
1a3b5f2
Compare
1a3b5f2 to
08cec22
Compare
08cec22 to
a494b82
Compare
3 participants
What changed?
Allow updating priority of Workflow and Activity.
Based on
Why?
Users want to change the priority after starting the workflow/activity.
How did you test it?