Use new API's simpler representation of versioning data #3432

Sushisource · 2022-09-27T00:44:02Z

What changed?
Handle changes in temporalio/api#237 which simplify the graph representation. Draft until that is merged (and sdk-go, etc).

Why?
Simpler better

How did you test it?
Changed all existing tests which still made sense, dropped nonsense ones, added new ones.

Potential risks
None, not in use yet.

Is hotfix candidate?

service/frontend/workflow_handler.go

service/matching/taskQueueManager_test.go

dnr · 2023-03-10T02:09:16Z

service/matching/version_sets.go

+	if data == nil || data.GetVersionSets() == nil {
+		return nil
+	}
+	asBytes, err := data.Marshal()


this is technically not guaranteed to be deterministic. I'm not sure how much we care

Seems kinda important. Can I use these XXX_Marshall functions? Gogo proto's docs are less than clear. Or is there some other simple way to guarantee that without doing it all by hand?

I don't know if there's anything in the proto lib or generated code that guarantees it. But I think in practice we can rely on it being deterministic within a single binary (running on a single architecture), which is the situation we care about

dnr · 2023-03-10T02:13:25Z

service/matching/version_sets.go

+	}
+	sets := g.GetVersionSets()
+	startIndex := len(sets) - maxDepth
+	shortened := g.GetVersionSets()[startIndex:]


Suggested change

shortened := g.GetVersionSets()[startIndex:]

shortened := util.SliceTail(g.GetVersionSets(), maxDepth)

(package common/util)

or really maybe just inline this at the two sites? one function that does two kinda related but different things is a little icky

Used the util, but kept this. I just can't bring myself to copy-paste stuff lol.

Fixed this anyways since the linter whines about it

service/matching/version_sets.go

dnr · 2023-03-10T02:20:11Z

service/matching/version_sets.go

+		}
+
+		existingData.VersionSets = append(existingData.GetVersionSets(), &taskqueuepb.CompatibleVersionSet{
+			Versions: []string{targetedVersion},


need to generate a uuid here. I have it in my pending PR on top of this but you could add it here if you want

dnr · 2023-03-10T02:25:30Z

service/matching/version_sets.go

+}
+
+func extractTargetedVersion(req *workflowservice.UpdateWorkerBuildIdCompatabilityRequest) string {
+	if req.GetAddNewCompatibleVersion() != nil {


kinda weird to effectively do this type switch twice, and using a different technique too. I know go (nor proto) doesn't have the pattern matching that would make this nice, but maybe better to do one switch and just pull these out of the typed value? (i.e. drop this function entirely and just read the fields as required in the function above)

Problem is I also need to get it out first to pass it into findVersion. I've made the styles consistent now but I don't immediately see a non-super-duplicative way to get rid of this fn.

yeah, I meant duplicate calls to findVersion in each branch. it's more duplication in some ways (code in branches of type switch) and less in others (type switch itself). overall I'm guessing it comes out shorter. but it's subjective so 🤷

service/matching/version_sets.go

service/matching/version_sets_test.go

Sushisource · 2023-03-13T16:43:02Z

Will be merged after API/Go sdk changes

dnr

just stylistic stuff at this point so feel free to take the suggestions you like

dnr · 2023-03-15T01:44:26Z

go.mod

@@ -44,8 +44,8 @@ require (
 	go.opentelemetry.io/otel/metric v0.36.0
 	go.opentelemetry.io/otel/sdk v1.13.0
 	go.opentelemetry.io/otel/sdk/metric v0.36.0
-	go.temporal.io/api v1.18.1
-	go.temporal.io/sdk v1.21.1
+	go.temporal.io/api v1.19.0


in the future I don't think there's any reason to "release" the api repo, we can just use whatever commit

service/frontend/workflow_handler.go

dnr · 2023-03-15T02:02:04Z

service/matching/version_sets.go

+	if data == nil || data.GetVersionSets() == nil {
+		return nil
+	}
+	asBytes, err := data.Marshal()


I don't know if there's anything in the proto lib or generated code that guarantees it. But I think in practice we can rely on it being deterministic within a single binary (running on a single architecture), which is the situation we care about

service/matching/version_sets.go

dnr · 2023-03-15T02:08:13Z

service/matching/version_sets.go

+	}
+	// Limit graph size if it's grown too large
+	newResp := depthLimiter(existingData, maxSize)
+	existingData.VersionSets = newResp.GetMajorVersionSets()


creating a "response" struct in the function to just pull out one field is one reason I wanted to inline this. I still think that would be better, but it's subjective at this point

service/matching/version_sets.go

dnr · 2023-03-15T02:29:26Z

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 *persistence.VersioningData, buildID string) findVersionRes {


use multiple return values instead of struct?

Suggested change

func findVersion(data *persistence.VersioningData, buildID string) findVersionRes {

func findVersion(data *persistence.VersioningData, buildID string) (setIndex int, indexInSet int) {

The linter bitched at me for that lol

dnr · 2023-03-15T02:32:45Z

service/matching/version_sets.go

+}
+
+func extractTargetedVersion(req *workflowservice.UpdateWorkerBuildIdCompatabilityRequest) string {
+	if req.GetAddNewCompatibleVersion() != nil {


yeah, I meant duplicate calls to findVersion in each branch. it's more duplication in some ways (code in branches of type switch) and less in others (type switch itself). overall I'm guessing it comes out shorter. but it's subjective so 🤷

Sushisource mentioned this pull request Sep 27, 2022

Version graph fixes #3287

Closed

Sushisource force-pushed the versioning-api-refactor branch 2 times, most recently from 2d201a1 to de0fe81 Compare September 28, 2022 17:26

Sushisource force-pushed the versioning-api-refactor branch from 8614b5e to f6bb789 Compare October 26, 2022 21:48

Sushisource force-pushed the versioning-api-refactor branch from f6bb789 to f264d93 Compare February 23, 2023 00:44

sync-by-unito bot closed this Mar 3, 2023

yycptt reopened this Mar 3, 2023

dnr reviewed Mar 10, 2023

View reviewed changes

Sushisource force-pushed the versioning-api-refactor branch from deaaf56 to 0087277 Compare March 10, 2023 17:58

Sushisource marked this pull request as ready for review March 13, 2023 16:42

Sushisource requested a review from a team as a code owner March 13, 2023 16:42

Sushisource force-pushed the versioning-api-refactor branch 6 times, most recently from 3830d5f to dae80d0 Compare March 14, 2023 16:18

Sushisource added 7 commits March 14, 2023 15:50

Updates to account for API refactoring

15dc258

Implement promote-within-set operation

cfe4b9a

Deal with renames

74501af

Fix remaining stuff

430a29e

Address David's review

e559ce8

Fix temporary go mod paths & apply renames

fb70007

Lint fix

27ca14f

Sushisource force-pushed the versioning-api-refactor branch from dae80d0 to 27ca14f Compare March 14, 2023 22:51

dnr approved these changes Mar 15, 2023

View reviewed changes

Last few style fixes

b4d1e37

Sushisource enabled auto-merge (squash) March 15, 2023 16:32

Sushisource merged commit 07ffab5 into temporalio:master Mar 15, 2023
7 checks passed

Sushisource deleted the versioning-api-refactor branch March 15, 2023 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new API's simpler representation of versioning data #3432

Use new API's simpler representation of versioning data #3432

Sushisource commented Sep 27, 2022

dnr Mar 10, 2023

Sushisource Mar 10, 2023

dnr Mar 15, 2023

dnr Mar 10, 2023

Sushisource Mar 10, 2023

Sushisource Mar 13, 2023

dnr Mar 10, 2023

Sushisource Mar 10, 2023

dnr Mar 10, 2023 •

edited

Sushisource Mar 10, 2023

dnr Mar 15, 2023

Sushisource commented Mar 13, 2023

dnr left a comment

dnr Mar 15, 2023

dnr Mar 15, 2023

dnr Mar 15, 2023

dnr Mar 15, 2023

Sushisource Mar 15, 2023

dnr Mar 15, 2023

	shortened := g.GetVersionSets()[startIndex:]
	shortened := util.SliceTail(g.GetVersionSets(), maxDepth)

	func findVersion(data *persistence.VersioningData, buildID string) findVersionRes {
	func findVersion(data *persistence.VersioningData, buildID string) (setIndex int, indexInSet int) {

Use new API's simpler representation of versioning data #3432

Use new API's simpler representation of versioning data #3432

Conversation

Sushisource commented Sep 27, 2022

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

dnr Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource commented Mar 13, 2023

dnr 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

dnr Mar 10, 2023 •

edited