Initial protos for priority #513

dnr · 2025-01-14T23:29:54Z

Note this is a feature branch PR

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.

Sushisource

Neat! Exciting!

Sushisource · 2025-01-14T23:39:36Z

temporal/api/common/v1/message.proto

+    // 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.
+    int32 priority = 1;


I know opinions differ on this matter (thanks, Go) but, I'd be for a uint here.

Yeah.. my rule is only use uint when you're talking about bit strings and not interpreting the value as an integer. So I want to use signed int here (and in code).

I'd consider a float, heh. Though I am ok with integers. But agree, unlike Rust and some low-level langs, in proto and most others it's "int unless you have a really good reason not to".

This definitely cannot be a float. The whole point is that there is a very limited number of options (so we can basically create a queue per value). Otherwise the whole setup of priority over fairness over ordering doesn't work.

Sushisource · 2025-01-14T23:42:20Z

temporal/api/common/v1/message.proto

+// that a workflow with a non-empty-string fairness key can't override the
+// fairness key of an activity to the empty string, it will be interpreted as
+// "inherit the workflow's key".


Could make fairness_key optional to avoid that ambiguity

Server (and go sdk) are using proto3 which can't distinguish empty string from not present, so it wouldn't really help. We can move to the new proto editions eventually but I don't think it's happening any time soon.

That should be totally possible in proto3, you'll get a *string instead of just string.

Of course I tried that in the API Go repo and it poops because of some lame generator limitation:

temporal/api/workflowservice/v1/request_response.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-grpc-gateway hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional.

So, fine, but, it's not a proto3 limitation: https://protobuf.dev/reference/go/go-generated/#singular-scalar-proto3

Ok, true.

It looks like grpc-gateway has been fixed (https://www.github.com/grpc-ecosystem/grpc-gateway/pull/1951), we just need to upgrade.

We can do it later, though, so I'd rather not block this on upgrading that.

temporal/api/common/v1/message.proto

Sushisource · 2025-01-14T23:47:28Z

temporal/api/common/v1/message.proto

+    // As with weights, rate limits can come from different sources:
+    // 1. Rate limits for a small set of keys can be overridden in task queue
+    //    configuration with an API.
+    // 2. It can be attached to the workflow/activity/task in this field.


For both this bullet and the same one above, I'm not sure I understand. The weight/limit "coming from" being attached to a task doesn't really make sense to me since I would think of it being attached to a task after it's been determined from some other source, since tasks are an output, not an input (except to workers, but, workers aren't doing this stuff).

Is it meant to mean that it's attached to a task at some earlier point within server, like when being forwarded between partitions or something? Would be good to elaborate a bit.

That's supposed to be "attached to the workflow" or "attached to the activity". If I just remove "/task" is that clearer?

I added "/task" because it could eventually be attached to nexus tasks and other things, but maybe we just shouldn't even mention that at this point.

The problem we're trying to solve is:

There's no limit on cardinality of fairness keys.

We're willing to store overrides for only a limited number (e.g. 100) fairness keys in task queue configuration.

Users may want to set a distinct weight or rate limit on more than 100 keys.

So we allow just including the weight+rate limit in the priority message itself, essentially using workflow/activity/task metadata to store more than 100 keys, except they're distributed around the system so it's scalable. We don't promise to remember and act on an unlimited number of distinct values, but we can try to act on the ones that we see, as we see them, as best we can.

Yeah I think removing task makes it more clear. Or just elaborate a bit to say that the value originates from the workflow/activition/operation invocation and thus ends up attached to the task.

Sushisource · 2025-01-14T23:50:54Z

temporal/api/workflowservice/v1/request_response.proto

+    // Priority metadata
+    temporal.api.common.v1.Priority priority = 18;


Does it not also need to be on poll WFT response? Or is it being in the workflow started event sufficient?

Yeah that was my thought, the sdk can track the priority metadata of the workflow from the started event (plus workflow properties modified events when we add it there later)

Co-authored-by: Spencer Judge <sjudge@hey.com>

bergundy · 2025-01-15T00:38:25Z

temporal/api/history/v1/message.proto

@@ -133,6 +133,8 @@ message WorkflowExecutionStartedEventAttributes {
    string inherited_build_id = 32;
    // Versioning override applied to this workflow when it was started.
    temporal.api.workflow.v1.VersioningOverride versioning_override = 33;
+    // Priority metadata


nit: Can you please be consistent with your use of punctuation?

Suggested change

// Priority metadata

// Priority metadata.

The rule I use (very consistently) is: comments that contain full sentences get periods, comments that are only sentence fragments don't.

I didn't just make this up, it basically matches the recommendation here, for example: https://google.github.io/styleguide/go/decisions#comment-sentences

However, I can use a different rule if you like.

FWIW, this is the rule I follow quite religiously in code and is a good one, it differentiates the "phrases" from the multi-sentence paragraphs.

I tend to prefer to use punctuation and proper capitalization in all doc strings. It's more consistent and prevents cases where the author adds more sentences later and forgets to add punctuation to the initial comment.

I switched from y'all's convention to this one but I'm obviously not blocking the PR for my personal preference.
Wondering what others think.

bergundy · 2025-01-15T00:42:01Z

temporal/api/common/v1/message.proto

+//
+// Not all queues in the system may support the "full" semantics of all priority
+// fields. (Currently only support in matching task queues is planned.)
+message Priority {


Wondering if there's a better name for this than Priority since that's just one aspect of this message.
Maybe something like SchedulingPolicy?

Yeah, I went through a few names, and I agree it's confusing that "priority" the name of the message refers to a larger concept than "priority" the field. But I also want a concise name here. "PriorityFairnessMetadata" is accurate but too long.

SchedulingPolicy suggests something slightly different to me. This isn't a "policy", it's metadata to inform prioritization decisions. Let's keep coming up with ideas though.

Could also be TaskDispatchStrategy or something similarly obtuse heh. But I'm ok with "priority" (though I do wonder if it needs to be qualified, see my other comment)

I want a short simple name, not an obtuse one. In the standup today we discussed this and one popular option was to keep this named "Priority" and change the name of the field to "priority key" to differentiate it.

bergundy · 2025-01-15T00:43:57Z

temporal/api/common/v1/message.proto

+    string fairness_key = 2;
+
+    // Fairness weight for a task can come from multiple sources for
+    // flexibility. From highest to lowest precedence:
+    // 1. Weights for a small set of keys can be overridden in task queue
+    //    configuration with an API.
+    // 2. It can be attached to the workflow/activity in this field.
+    // 3. The default weight of 1.0 will be used.
+    // Note that if the weight for a key is attached in this field, for best
+    // results, the same weight should be used for the same key for a reasonable
+    // amount of time (minutes). It may change, but it may take some time for
+    // the change to be reflected.
+    //
+    // The recommended range of usable weights is [0.001, 1000].
+    float fairness_weight = 3;
+
+    // The fairness mechanism can also enforce rate limits per fairness key.
+    // Rate limits are specified in tasks dispatched per second.
+    // As with weights, rate limits can come from different sources:
+    // 1. Rate limits for a small set of keys can be overridden in task queue
+    //    configuration with an API.
+    // 2. It can be attached to the workflow/activity in this field.
+    // 3. The "default" rate limit for keys that are not overridden can be set
+    //    in task queue configuration also.
+    // 4. Otherwise, a very high rate limit will be applied.
+    float fairness_rate_limit = 4;
+
+    // Ordering key is a positive integer from 1 to MaxInt64. After priority and
+    // fairness mechanisms are applied, tasks will be finally ordered by
+    // ordering_key. Note that fine-grained values are allowed here, as opposed
+    // to priority.
+    //
+    // For example, applications might use the start time of a workflow (in
+    // seconds since the unix epoch) as ordering key for the workflow and all
+    // activities in it. This will have the effect of prioritizing activities of
+    // workflows that were started earlier, encouring completing older workflows
+    // over making progress in newer ones.
+    int64 ordering_key = 5;


Do we want to add all of these definitions before they're available?

I think that's fine if you're not merging to main but generally, we shouldn't expose fields that aren't yet supported to avoid confusing consumers of the API and have the ability to iterate on the API as the implementation progresses.

I'll defend David here: ✨ read the description ✨

I'll comment out/delete the rest before merging and add them back later.

I said in both the PR description and on slack that things will only be merged to main as implemented :)

The full version is presented here to give context to sdk team when considering how to expose this in sdks.

Question, though: for the parts that aren't implemented yet, is commenting out the fields enough, or should we delete everything?

I would delete. I realize that the target of this PR isn't the main branch but still wanted to ensure my position was clear.

I'll merge this as-is to keep a reference to point to, and delete on the feature branch before merging

cretz

Nothing blocking, especially since this is going to a feature branch

cretz · 2025-01-15T13:58:02Z

temporal/api/common/v1/message.proto

+    // 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.
+    int32 priority = 1;


I'd consider a float, heh. Though I am ok with integers. But agree, unlike Rust and some low-level langs, in proto and most others it's "int unless you have a really good reason not to".

cretz · 2025-01-15T14:01:23Z

temporal/api/common/v1/message.proto

+    // activities in it. This will have the effect of prioritizing activities of
+    // workflows that were started earlier, encouring completing older workflows
+    // over making progress in newer ones.
+    int64 ordering_key = 5;


Minor nit, would consider calling this ordering_value or something as _key has a map-key feeling akin to fairness_key

I get that, though "value" feels too generic. This basically corresponds to the concept of "clustering key" in Cassandra, which uses "key", so it feels okay to me.

cretz · 2025-01-15T14:02:00Z

temporal/api/history/v1/message.proto

@@ -133,6 +133,8 @@ message WorkflowExecutionStartedEventAttributes {
    string inherited_build_id = 32;
    // Versioning override applied to this workflow when it was started.
    temporal.api.workflow.v1.VersioningOverride versioning_override = 33;
+    // Priority metadata


FWIW, this is the rule I follow quite religiously in code and is a good one, it differentiates the "phrases" from the multi-sentence paragraphs.

cretz · 2025-01-15T14:05:49Z

temporal/api/common/v1/message.proto

+// Note that for all fields other than fairness_key, the not-present/zero value
+// is invalid, but for fairness_key, the empty string is a valid key. This means


Suggested change

// Note that for all fields other than fairness_key, the not-present/zero value

// is invalid, but for fairness_key, the empty string is a valid key. This means

// Note that for all fields other than fairness_key, the not-present/zero value

// is invalid when returned from the server, but for fairness_key, the empty string is a valid key. This means

I was a bit confused seeing absent/zero as invalid here and the paragraph above talking about absent/zero fields

I don't think that quite captures my meaning. It's more about commands, actually: say you start an activity (or child wf) and want to override the priority, but not the fairness key. I'm imagining the sdk would attach a Priority message that includes a non-zero priority, and a zero (empty string) fairness key. The server would interpret that as: override the priority, inherit the fairness key from the workflow.

That is, you can override any individual field(s) by setting it to a non-zero value. That works well in all cases except one: you can't override the fairness key to the empty string. I think that limitation is fine, since that's a pretty weird thing to want to do: if you're using fairness keys at all, you want to set it explicitly in all cases and not mix the default (empty string) with other values.

That is, you can override any individual field(s) by setting it to a non-zero valu

So you can provide this object as partial not-present/zero values to inherit them? The statement "for all fields other than fairness_key, the not-present/zero value is invalid" seems to say I have to provide every field but fairness key each time I make this object (like when putting on a command and only wanting to set priority and inherit the rest)?

By "the zero value is invalid", I mean that zero is not meaningful if interpreted as a value, and therefore we can be absolutely sure that the intention was "default/inherit". Not that it's not acceptable to send this message with missing/zero values. On the contrary, it's always acceptable to leave out the whole message or any subset of fields. (It has to be so for backwards compatibility)

For fairness key, the zero value is meaningful if interpreted as a value, but I'm saying we don't interpret it that way, we interpret it as "inherit".

There may be some confusion here because in some contexts (schedule activity command) there is a "parent" to inherit from, and in other contexts (start workflow execution) there is no "parent", so you can't "inherit", you just get the default values. The same message is used in both cases though, since it's basically the same thing, it's just that the "parent" is the default values.

By "the zero value is invalid", I mean that zero is not meaningful if interpreted as a value

Ah, I misinterpreted "invalid" to mean "wouldn't pass validation" and would therefore error, meaning it's an invalid thing for SDK side to ever set (or leave unset).

I'll reword

cretz · 2025-01-15T14:07:27Z

temporal/api/workflow/v1/message.proto

@@ -110,6 +110,9 @@ message WorkflowExecutionInfo {
    // be versioned or unversioned, depending on `versioning_info.behavior` and `versioning_info.versioning_override`.
    // Experimental. Versioning info is experimental and might change in the future.
    WorkflowExecutionVersioningInfo versioning_info = 22;
+
+    // Priority metadata
+    temporal.api.common.v1.Priority priority = 23;


To confirm, this is intentionally in visibility as opposed to putting in WorkflowExecutionExtendedInfo?

Good question. It was intentional but I'm not totally confident. I do think it can be valuable to have this info available through visibility, but I'm not sure of all the implications

After some discussion we tentatively agreed to keep this here

cretz · 2025-01-15T14:08:27Z

temporal/api/common/v1/message.proto

+// matching (workflow and activity) task queues. Later it may be used in history
+// task queues and in rate limiting decisions.
+//
+// Priority is attached to workflows, activities, and Nexus tasks. By default,


and Nexus tasks

I don't see that in this PR, is that maybe coming later?

You're right, I decided we shouldn't do anything with nexus pending more design discussion

cretz · 2025-01-15T22:14:42Z

temporal/api/common/v1/message.proto

+//
+// Not all queues in the system may support the "full" semantics of all priority
+// fields. (Currently only support in matching task queues is planned.)
+message Priority {


I wonder if this needs a qualifier since it's in the common package, e.g. TaskPriority, or if it should move to the taskqueue package.

The initial application will be in matching task queues, but we can eventually use this in other contexts, for example attaching priorities to any api to influence load shedding at capacity, or attaching priorities to signals or timers (which do turn into tasks at some point but conceptually aren't really just tasks).

Yeah, my concern is pedantic, but so often these assumptions of generic reuse don't materialize due to some minor mismatch need for the other use case. But can keep it as generically named for all concepts of Temporal "priority" ever if we want.

I had a conversation yesterday about how this can fit into the current flow control work and it seems promising, so I'm really hoping we can get some reuse here. We can reconsider at the feature branch merge.

**What changed?** Add "activity_paused" flag to RecordActivityTaskHeartbeatByIdResponse.  **Why?** 1. It is needed to let long-running activity know that activity was paused on the server. 2. Because it is missing in RecordActivityTaskHeartbeatByIdResponse but present in RecordActivityTaskHeartbeatResponse  **Breaking changes** No.

**What changed?** GHA to create, publish and delete releases. - `create-release`: manually triggered, it validates the input, checks compatibility with sdk-go, and creates draft releases in `api` and `api-go`. - `publish-release`: triggered when a release is published, it publishes the corresponding `api-go` release. - `delete-release`: triggered when a release is deleted, it deletes the corresponding `api-go` release (only works for deleting draft releases).  **Why?** Automation.  **Breaking changes**  **Server PR**

…Attributes (temporalio#516) _**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: Add unset_versioning_override to WorkflowExecutionOptionsUpdatedEventAttributes  ### Why: So that users of this event don't need to load VersioningOverride from mutable state every time they create this event. This change was prompted because the event is now being used for non-version-override-related things, and I received feedback that it is inefficient / awkward / error-prone to have to load and pass in the current versioning override every time anyone writes to this event. Now, a nil Versioning Override in this event means "no change" instead of "remove". This reduces the chance that someone accidentally unsets an override in the future, and also is more efficient. We've discussed this change internally in the server team and are ok with changing the meaning of this history event, because it is such a small change and the scope of impact is small (pre-release versioning users who have unset a versioning override and are building mutable state from that history).  ### Breaking changes? Now, a nil Versioning Override in this event means "no change" instead of "remove". If an event exists with the previous meaning and the mutable state is rebuilt, the Versioning Override would not be removed. But the chance of that happening is very low.  temporalio/temporal#7091

**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>

dnr added 2 commits January 14, 2025 15:14

common message

d0a484c

add priority to a bunch of messages

99077be

dnr requested review from a team as code owners January 14, 2025 23:29

Sushisource reviewed Jan 14, 2025

View reviewed changes

dnr and others added 2 commits January 14, 2025 16:07

Apply suggestions from code review

57b033a

Co-authored-by: Spencer Judge <sjudge@hey.com>

remove /task

cdc4336

bergundy approved these changes Jan 15, 2025

View reviewed changes

cretz approved these changes Jan 15, 2025

View reviewed changes

remove nexus mention

51f90e8

cretz reviewed Jan 15, 2025

View reviewed changes

dnr and others added 8 commits January 15, 2025 15:57

priority key

310a73e

generated stuff

13b73e4

reword

d2786b5

Merge branch 'master' of github.com:temporalio/api into pri1

441e372

generate

2835657

dnr merged commit 9649d4c into temporalio:priority Jan 18, 2025
2 checks passed

stephanos mentioned this pull request Jan 27, 2025

Add priority annotations temporalio/sdk-go#1792

Open

13 tasks

		// Priority metadata
		temporal.api.common.v1.Priority priority = 18;

		// Note that for all fields other than fairness_key, the not-present/zero value
		// is invalid, but for fairness_key, the empty string is a valid key. This means

Initial protos for priority #513

Initial protos for priority #513

Conversation

dnr commented Jan 14, 2025

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

Sushisource Jan 15, 2025 • 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

cretz Jan 15, 2025 • 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

cretz left a comment • 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

cretz Jan 15, 2025 • 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

cretz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Jan 15, 2025 •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

cretz left a comment •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

cretz Jan 16, 2025 •

edited

Loading