Update Workflow and Activity Priority #610
Conversation
stephanos
force-pushed
the
update-prio
branch
2 times, most recently
from
626b76b to
ab563c6
Compare
|
|
||
|
|
||
| // Priority metadata | ||
| // Deprecated. Use `activity_options.priority`. |
There was a problem hiding this comment.
This is a unfortunate since I only now realized that PendingActivityInfo contains ActivityOptions but we've already added temporal.api.common.v1.Priority priority = 22; there ... 😞
There was a problem hiding this comment.
We need to make sure this value keeps getting written by server for a while.
Should also use [deprecated = true].
Alternatively, related to my other comment, maybe we clarify that this field is always the original priority, and activity_options has the override, if any?
| // Overrides the activity's priority sent by the SDK. | ||
| temporal.api.common.v1.Priority priority = 7; |
There was a problem hiding this comment.
This field has two meanings, one when writing, one when reading. This comment only describes the write behavior.
The read behavior might be an override or the original.
There was a problem hiding this comment.
I like that! I'll change it to that.
There was a problem hiding this comment.
@dnr we could lean even more into this and make the field in PendingActivityInfo always represent the user set value and ActivityOptions to contain the latest value. Meaning, when it inherits the workflow's priority, the data would show that the user set no priority but the actual priority was the one from the workflow (or a manual override).
Semantically, this would be a breaking change. Maybe fine in pre-release, though.
There was a problem hiding this comment.
I sorta like the idea of differentiating what was explicitly set in on the activity from what it inherited from the workflow... and from what it was later overridden to? There are really four values: 1. inherited from workflow, 2. explicitly set on activity by workflow, 3. overridden by update call, and 4. "effective" priority which is the merged value of the first three in that order.
But it's not obvious to me why PendingActivityInfo would have one of those and ActivityOptions would have another. I mean, if you asked me I probably would have said ActivityOptions should have the explicit set value and PendingActivityInfo would have the "effective" one (backwards from what you said I think).
So if we really want to split them out, maybe we should include multiple Priorities in each of those locations? But before we get into that complexity, let's consider if it's really worth it. Why do users want to read this value and which one are they more interested in?
The most intuitive to me is: this field is 2+3 in my initial list: the explicitly-set Priority, which can be changed by update call. The "effective" value is not reported explicitly anywhere. But I could easily be convinced otherwise.
There was a problem hiding this comment.
Coming back here, I think we have two options:
(a) Deprecate the field in PendingActivityInfo; keep the field in ActivityOptions. Until we can remove it, hydrate both from the server with the explicitly set data (ie 2+3); ie status quo.
(b) As proposed above, make the ActivityOptions reflect the explicitly set data (ie 2+3); and then I'd argue that realistically leaves only (4) for PendingActivityInfo since (1) seems not helpful.
I like (b) only because it would allow us to easily show what the effective priority options are in the CLI/UI. That in itself seems useful to communicate. But that would technically be a breaking behavior change; although, I don't think anyone relies on this (not sure it's exposed?). And it's pre-release.
If we don't find that valuable (or possibly confusing; the biggest risk here AFAICT), we still have option (a) since the feature is in pre-release.
|
|
||
|
|
||
| // Priority metadata | ||
| // Deprecated. Use `activity_options.priority`. |
There was a problem hiding this comment.
We need to make sure this value keeps getting written by server for a while.
Should also use [deprecated = true].
Alternatively, related to my other comment, maybe we clarify that this field is always the original priority, and activity_options has the override, if any?
| // Overrides the activity's priority sent by the SDK. | ||
| temporal.api.common.v1.Priority priority = 7; |
There was a problem hiding this comment.
I sorta like the idea of differentiating what was explicitly set in on the activity from what it inherited from the workflow... and from what it was later overridden to? There are really four values: 1. inherited from workflow, 2. explicitly set on activity by workflow, 3. overridden by update call, and 4. "effective" priority which is the merged value of the first three in that order.
But it's not obvious to me why PendingActivityInfo would have one of those and ActivityOptions would have another. I mean, if you asked me I probably would have said ActivityOptions should have the explicit set value and PendingActivityInfo would have the "effective" one (backwards from what you said I think).
So if we really want to split them out, maybe we should include multiple Priorities in each of those locations? But before we get into that complexity, let's consider if it's really worth it. Why do users want to read this value and which one are they more interested in?
The most intuitive to me is: this field is 2+3 in my initial list: the explicitly-set Priority, which can be changed by update call. The "effective" value is not reported explicitly anywhere. But I could easily be convinced otherwise.
| string attached_request_id = 3; | ||
| // Completion callbacks attached to the running workflow execution. | ||
| repeated temporal.api.common.v1.Callback attached_completion_callbacks = 4; | ||
| temporal.api.common.v1.Priority priority = 5; |
There was a problem hiding this comment.
Maybe a comment here? Does this contain the full new Priority or only the changed fields?
There was a problem hiding this comment.
👍 I've added a comment now
Since this is for the workflow options, it's valid to allow zero values in the update (e.g. for fairness key). Therefore we must store the full priority message even if the user only updated one field to disambiguate between default/not changed and a change.
3 participants
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
Added
Priorityfield to the workflow and activity update API options.Batch API support is going to be added in a follow-up PR.
Why?
Allow users to update priority after the workflow/activity was submitted.
Breaking changes
See field deprecation.
Server PR
TBD