-
Notifications
You must be signed in to change notification settings - Fork 75
Merge initial protos for priority #561
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
Conversation
**What changed?** Add initial protos for priority, enough to attach priority metadata to workflows and activities. Later, we'll add APIs to update priority of existing workflows and activities, to override values in task queue configuration, and observability APIs. This has the full planned `Priority` message to provide context for reviewers, even though the implementation will come slowly over time. Specifically, in the first phase only the `priority` field will be meaningful. I'll comment out/delete the rest before merging and add them back later. **Why?** Control task ordering. --------- Co-authored-by: Spencer Judge <sjudge@hey.com> Co-authored-by: Yuri <ychebotarev@gmail.com> Co-authored-by: Rodrigo Zhou <2068124+rodrigozhou@users.noreply.github.com> Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>
**What changed?** Remove future planned fields from Priority message for now. **Why?** Prepare for merging to main. Only priority_key is implemented so far.
Co-authored-by: Spencer Judge <spencer@temporal.io>
temporal/api/common/v1/message.proto
Outdated
// If priority is not present (or zero), then the effective priority will be | ||
// the default priority, which is is calculated by (min+max)/2. With the | ||
// default max of 5, and min of 1, that comes out to 3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If priority is not present (or zero), then the effective priority will be the default priority, which is is calculated by (min+max)/2. With the default max of 5, and min of 1, that comes out to 3.
This is a bit in conflict (or at least confusing) with the comment above the message which states:
For all fields, the field not present or equal to zero/empty string means to inherit the value from the calling workflow, or if there is no calling workflow, then use the default value.
Should the first quoted statement change "If priority is not present (or zero)" to "If priority is not present (or zero) and there is no calling workflow"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the confusion: the above comment is talking about how to merge these messages if there's one on the workflow and also an activity call (or child wf), the one here is talking from the perspective of a post-merged message and saying what happens if it's still zero. I'll try to reword.
What changed?
Add initial protos for priority, enough to attach simple priority metadata to workflows and activities.
See #513 for the full future plans, only the first field (priority_key) is included so far.
Why?
Control task ordering.
Server PR
Implementation on
priority
branch: temporalio/temporal@main...priority