Consider non default sets reachable by new workflows for a while after they stop being queue default #4545

bergundy · 2023-06-26T21:15:48Z

What changed?

See title, also added a frontend.reachabilityQuerySetDurationSinceDefault dynamic config with a default of 5 minutes.

Why?

Docstring has an explanation:

	// ReachabilityQuerySetDurationSinceDefault is the minimum period since a version set was demoted from being the
	// queue default before it is considered unreachable by new workflows.
	// This setting allows some propogation delay of versioning data for the reachability queries, which may happen for
	// the following reasons:
	// 1. There are no workflows currently marked as open in the visibility store but a worker for the demoted version
	// is currently processing a task.
	// 2. There are delays in the asynchrnous visiblity task processor.
	// 3. There's propagation delay of the versioning data between matching nodes.

How did you test it?

Added a couple of tests

common/dynamicconfig/constants.go

dnr · 2023-06-28T20:15:10Z

common/dynamicconfig/constants.go

 	// ReachabilityQueryBuildIdLimit limits the number of build ids that can be requested in a single call to the
 	// GetWorkerTaskReachability API.
 	ReachabilityQueryBuildIdLimit = "limit.reachabilityQueryBuildIds"
+	// ReachabilityQuerySetDurationSinceDefault is the minimum period since a version set was demoted from being the


the two configs represent the same concept, right? why not collapse them?

Removing a build id is more severe, I think we want different configs for them.

ok, that's fair

dnr · 2023-06-28T20:16:24Z

common/util/util.go

 }
+
+// ReduceSliceInitial reduces a slice using given reducer function and initial value.
+func ReduceSliceInitial[T any, A any](in []T, initializer A, reducer func(A, T) A) A {


Suggested change

func ReduceSliceInitial[T any, A any](in []T, initializer A, reducer func(A, T) A) A {

func ReduceSlice[T any, A any](in []T, initializer A, reducer func(A, T) A) A {

Reduce always needs an initial value, no need to include it in the name

There's also reduce from T[] to T that can use the first element as the initializer. I didn't add it to our "godash" lib though.

I'd rather force callers to do reduce(slice[1:], slice[0], f) so then it's clearly the caller's responsibility to do something for the empty slice

dnr · 2023-06-28T20:25:33Z

service/frontend/task_reachability.go

-					Namespace: ns.Name().String(),
-					TaskQueue: taskQueue,
-				},
+			value, err := f.matchingClient.GetTaskQueueUserData(ctx, &matchingservice.GetTaskQueueUserDataRequest{


do you think it makes sense to use the more general GetTaskQueueUserData in frontend's GetWorkerBuildIdCompatibility also and remove that specific rpc from matching? it's not really doing anything besides ToBuildIdOrderingResponse, which frontend could call.

Sure, that makes sense to me.

TBH, I would just send all of the user facing user data info in DescribeTaskQueue and not have a special API for GetWorkerBuildIdCompatibility.

In any case, I wouldn't change in this PR.
We'd need to leave the matching RPC for this minor version because 1.21 was already released.

you mean not have GetWorkerBuildIdCompatibility in workflowservice (frontend)? I like that but we released it already, can we remove it? for matchingservice we need a separate GetTaskQueueUserData for long polls anyway so might as well use that

We marked this whole feature as experimental (API-wise), we can still change but we'll likely need to deprecate APIs and keep them around for a while since users will start relying on them fairly soon.

dnr · 2023-06-28T20:26:58Z

service/frontend/task_reachability.go

-				},
+			value, err := f.matchingClient.GetTaskQueueUserData(ctx, &matchingservice.GetTaskQueueUserDataRequest{
+				NamespaceId:   ns.ID().String(),
+				TaskQueue:     taskQueue,


this always goes to the root (it did before too). probably okay but we could consider load-balancing this

I'm more concerned with hammering visibility than getting this info from matching but I do see your point.
Do you think it's worth it?

nah, you're right this should be cheap compared to visibility

dnr · 2023-06-28T20:31:37Z

service/matching/version_sets.go


-// Finds the version in the version sets, returning (set index, index within that set)
-// Returns -1, -1 if not found.
-func findVersion(data *persistencespb.VersioningData, buildID string) (setIndex, indexInSet int) {


dnr · 2023-06-28T21:27:30Z

common/dynamicconfig/constants.go

 	// ReachabilityQueryBuildIdLimit limits the number of build ids that can be requested in a single call to the
 	// GetWorkerTaskReachability API.
 	ReachabilityQueryBuildIdLimit = "limit.reachabilityQueryBuildIds"
+	// ReachabilityQuerySetDurationSinceDefault is the minimum period since a version set was demoted from being the


ok, that's fair

dnr · 2023-06-28T21:29:04Z

common/util/util.go

 }
+
+// ReduceSliceInitial reduces a slice using given reducer function and initial value.
+func ReduceSliceInitial[T any, A any](in []T, initializer A, reducer func(A, T) A) A {


I'd rather force callers to do reduce(slice[1:], slice[0], f) so then it's clearly the caller's responsibility to do something for the empty slice

dnr · 2023-06-28T21:32:00Z

service/frontend/task_reachability.go

-					Namespace: ns.Name().String(),
-					TaskQueue: taskQueue,
-				},
+			value, err := f.matchingClient.GetTaskQueueUserData(ctx, &matchingservice.GetTaskQueueUserDataRequest{


you mean not have GetWorkerBuildIdCompatibility in workflowservice (frontend)? I like that but we released it already, can we remove it? for matchingservice we need a separate GetTaskQueueUserData for long polls anyway so might as well use that

dnr · 2023-06-28T21:32:48Z

service/frontend/task_reachability.go

-				},
+			value, err := f.matchingClient.GetTaskQueueUserData(ctx, &matchingservice.GetTaskQueueUserDataRequest{
+				NamespaceId:   ns.ID().String(),
+				TaskQueue:     taskQueue,


nah, you're right this should be cheap compared to visibility

dnr · 2023-06-28T21:34:35Z

common/dynamicconfig/constants.go

 	// 1. There are no workflows currently marked as open in the visibility store but a worker for the demoted version
 	// is currently processing a task.
-	// 2. There are delays in the asynchrnous visiblity task processor.
+	// 2. There are delays in the visibility task processor (which is asynchronous).


oh, can you make the same change on the copied line (214)?

…r they stop being queue default

Co-authored-by: David Reiss <[email protected]>

…r they stop being queue default (#4545)

bergundy requested a review from dnr June 26, 2023 21:15

bergundy requested a review from a team as a code owner June 26, 2023 21:15

dnr reviewed Jun 28, 2023

View reviewed changes

dnr approved these changes Jun 28, 2023

View reviewed changes

dnr reviewed Jun 28, 2023

View reviewed changes

bergundy and others added 3 commits June 28, 2023 20:58

Consider non default sets reachable by new workflows for a while afte…

bad031c

…r they stop being queue default

Apply suggestions from code review

d28e7ec

Co-authored-by: David Reiss <[email protected]>

Address the rest of the review comments

4a826ce

bergundy force-pushed the reachability-demotion-period branch from 31a4aaa to 4a826ce Compare June 29, 2023 04:04

bergundy enabled auto-merge (squash) June 29, 2023 04:05

bergundy merged commit c0fb121 into temporalio:master Jun 29, 2023

yiminc added the release/1.21.1 label Jun 29, 2023

mindaugasrukas pushed a commit that referenced this pull request Jun 29, 2023

Consider non default sets reachable by new workflows for a while afte…

ca41bb0

…r they stop being queue default (#4545)

	func ReduceSliceInitial[T any, A any](in []T, initializer A, reducer func(A, T) A) A {
	func ReduceSlice[T any, A any](in []T, initializer A, reducer func(A, T) A) A {

Consider non default sets reachable by new workflows for a while after they stop being queue default #4545

Consider non default sets reachable by new workflows for a while after they stop being queue default #4545

Uh oh!

Conversation

bergundy commented Jun 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnr Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dnr Jun 28, 2023 •

edited

Loading