Allow fairness weight overrides via UpdateTaskQueueConfigApi

[sivagirish81]

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 two update fairness weight maps to the UpdateTaskQueueConfig api request.
• Added a fairness_weights map in the taskQueueConfig message to enable persistence and return response.

Why?

• Allow override of fairness weights for individual fairness keys via UpdateTaskQueueConfig api.

Breaking changes

• No.

Server PR

[dnr]

I think this is simple and sufficient, I'd just tweak the names slightly.

Does anyone have objections to the "-1 to unset"?

[cretz]

(from a structure POV all LGTM, but am deferring SDK review to someone on SDK team more familiar w/ the priority/fairness project, maybe @Sushisource?)

[Sushisource]

Makes sense to me.

[cretz]

This is a bit rough. How do we expect unsetting an override to look in CLI, will we ask users to use -1? Arguably a separate list of keys to remove could be clearer, but I do not have strong opinion

[sivagirish81]

Addressing this here : https://github.com/temporalio/cli/pull/840/files#r2379720580

[stephanos]

Options I'm considering here now:

(1)

map<string, float> set_fairness_weight_overrides = 7;  // float must be zero or greater
repeated string unset_fairness_weight_overrides = 8;

(2)

message FairnessWeightUpdate {
  string key = 1;
  oneof op {
    float set = 2;
    bool unset = 3;      
  }
}

repeated FairnessWeightUpdate update_fairness_weight_overrides = 7;

(3) AI also suggested this to me. I didn't know proto3 actually had optional. Looks legit.

message FairnessWeightUpdate {
    optional float weight = 1;
}

map<string, FairnessWeightUpdate> update_fairness_weight_overrides = 7;

---

(1) seems clearest and easiest.

[cretz]

Agreed, 1 is clearest IMO. Would like to avoid optional and empty messages if we can. It is less clear to users to differentiate scalars between 0 and "none" (if optional even makes sense that way). People expect 0 and unset to be the same thing.

[sivagirish81]

Approach 1 looks good to me. A couple of points to consider for the server implementation:

• What should the expected behavior be if a user tries to set a fairness key as well as unset a fairness key in the same request. Do we do a sanity check and drop the request as a whole or do we prioritize the update.
• We should enforce a limit on the number of keys that can be deleted in a single request, similar to how we enforce a limit on the number of fairness weight overrides allowed at a time.
• Do we also want to support a mechanism for unsetting all overrides at once? This could be useful for reset scenarios.

[stephanos]

Good points to consider! I'd say

• "reject if a request contains conflicting requests"
• +1 to combined update limit
• could add a bool field (now validation becomes trickier) or declare out of scope for now

[cretz]

If other values in this request are unset, things remain as is. Are we inconsistently saying for this one field if you don't set it, we are going to revert to fall back values instead of leaving the overrides alone? Or do you mean "If unset" as in if a certain key override is not present at runtime, not whether this field is unset? It is a bit confusing looking at other fields in this request talk about "If not set" which does refer to the field, not the runtime behavior of the value.

[sivagirish81]

If other values in this request are unset, things remain as is. Are we inconsistently saying for this one field if you don't set it, we are going to revert to fall back values instead of leaving the overrides alone?

No. The same behavior applies for this field as well. If the values are not set in the api calls we will leave the overrides untouched.

Suggested change

	// If unset, the fairness weights fall back to the values set during task creation (if any),
	// or to the default weight of 1.0.
	// If unset via sentinel value, the fairness weights fall back to the values set during task creation (if any),
	// or to the default weight of 1.0.

If the value of a particular fairness key is unset in the map by using the sentinel value then the statement applies. I can see how it might be confusing.

[cretz]

(sorry, just noticed a couple of confusing things I commented on, does not have to be blocking since this was already reviewed)

[dnr]

Tbh I think the -1 thing is fine (for an api, not user-facing), but fine either way