From f264d93a9437854ff3056d946239008e06deba5c Mon Sep 17 00:00:00 2001 From: Spencer Judge Date: Wed, 22 Feb 2023 16:44:31 -0800 Subject: [PATCH] Fix remaining stuff --- service/frontend/workflow_handler.go | 39 +++++++++++--------- service/matching/matchingEngine.go | 2 +- service/matching/matchingEngine_test.go | 43 ++++++++-------------- service/matching/version_sets.go | 9 ++--- service/matching/version_sets_test.go | 47 +++++++++++++++---------- 5 files changed, 72 insertions(+), 68 deletions(-) diff --git a/service/frontend/workflow_handler.go b/service/frontend/workflow_handler.go index 33d77fb8948a..c9c6012b69fd 100644 --- a/service/frontend/workflow_handler.go +++ b/service/frontend/workflow_handler.go @@ -3613,7 +3613,7 @@ func (wh *WorkflowHandler) UpdateWorkerBuildIdCompatability(ctx context.Context, return nil, errRequestNotSet } - if err := wh.validateBuildIdOrderingUpdate(request); err != nil { + if err := wh.validateBuildIdCompatabilityUpdate(request); err != nil { return nil, err } @@ -4351,10 +4351,10 @@ func (wh *WorkflowHandler) validateTaskQueue(t *taskqueuepb.TaskQueue) error { return nil } -func (wh *WorkflowHandler) validateBuildIdOrderingUpdate( +func (wh *WorkflowHandler) validateBuildIdCompatabilityUpdate( req *workflowservice.UpdateWorkerBuildIdCompatabilityRequest, ) error { - errstr := "request to update worker build id ordering requires:" + errstr := "request to update worker build id compatability requires:" hadErr := false checkIdLen := func(id string) { @@ -4377,30 +4377,37 @@ func (wh *WorkflowHandler) validateBuildIdOrderingUpdate( errstr += " an operation to be specified." hadErr = true } - if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_NewCompatibleVersion_); ok { - if x.NewCompatibleVersion.GetNewVersionId() == "" { - errstr += " `new_version_id` to be set." + if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_AddNewCompatibleVersion_); ok { + if x.AddNewCompatibleVersion.GetNewVersionId() == "" { + errstr += " `add_new_compatible_version` to be set." hadErr = true } else { - checkIdLen(x.NewCompatibleVersion.GetNewVersionId()) + checkIdLen(x.AddNewCompatibleVersion.GetNewVersionId()) } - if x.NewCompatibleVersion.GetExistingCompatibleVersion() == "" { + if x.AddNewCompatibleVersion.GetExistingCompatibleVersion() == "" { errstr += " `existing_compatible_version` to be set." hadErr = true } - } else if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_NewDefaultVersionId); ok { - if x.NewDefaultVersionId == "" { - errstr += " `new_default_major_version_id` to be set." + } else if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_AddNewVersionIdInNewDefaultSet); ok { + if x.AddNewVersionIdInNewDefaultSet == "" { + errstr += " `add_new_version_id_in_new_default_set` to be set." hadErr = true } else { - checkIdLen(x.NewDefaultVersionId) + checkIdLen(x.AddNewVersionIdInNewDefaultSet) } - } else if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_ExistingVersionIdInSetToPromote); ok { - if x.ExistingVersionIdInSetToPromote == "" { - errstr += " `existing_version_id_in_set_to_promote` to be set." + } else if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_PromoteSetByVersionId); ok { + if x.PromoteSetByVersionId == "" { + errstr += " `promote_set_by_version_id` to be set." hadErr = true } else { - checkIdLen(x.ExistingVersionIdInSetToPromote) + checkIdLen(x.PromoteSetByVersionId) + } + } else if x, ok := req.GetOperation().(*workflowservice.UpdateWorkerBuildIdCompatabilityRequest_PromoteVersionIdWithinSet); ok { + if x.PromoteVersionIdWithinSet == "" { + errstr += " `promote_version_id_within_set` to be set." + hadErr = true + } else { + checkIdLen(x.PromoteVersionIdWithinSet) } } if hadErr { diff --git a/service/matching/matchingEngine.go b/service/matching/matchingEngine.go index af4f3cfc26d0..b0d7ac77a2ba 100644 --- a/service/matching/matchingEngine.go +++ b/service/matching/matchingEngine.go @@ -721,7 +721,7 @@ func (e *matchingEngineImpl) UpdateWorkerBuildIdCompatability( return nil, err } err = tqMgr.MutateVersioningData(hCtx.Context, func(data *persistencespb.VersioningData) error { - return UpdateVersionsGraph(data, req.GetRequest(), e.config.MaxVersionGraphSize()) + return UpdateVersionSets(data, req.GetRequest(), e.config.MaxVersionGraphSize()) }) if err != nil { return nil, err diff --git a/service/matching/matchingEngine_test.go b/service/matching/matchingEngine_test.go index c9e54d79b41f..92ba37c2a465 100644 --- a/service/matching/matchingEngine_test.go +++ b/service/matching/matchingEngine_test.go @@ -1978,14 +1978,13 @@ func (s *matchingEngineSuite) TestGetVersioningData() { }, }) s.NoError(err) - //s.NotNil(res.GetResponse().GetCurrentDefault()) - //lastNode := res.GetResponse().GetCurrentDefault() - //s.Equal(mkVerId("99"), lastNode.GetVersion()) - //for lastNode.GetPreviousIncompatible() != nil { - // lastNode = lastNode.GetPreviousIncompatible() - //} - //s.Equal(mkVerId("0"), lastNode.GetVersion()) - //s.Equal(mkVerId("99.9"), res.GetResponse().GetCompatibleLeaves()[0].GetVersion()) + majorSets := res.GetResponse().GetMajorVersionSets() + curDefault := majorSets[len(majorSets)-1] + s.NotNil(curDefault) + s.Equal("99", curDefault.GetVersions()[0]) + lastNode := curDefault.GetVersions()[len(curDefault.GetVersions())-1] + s.Equal("99.9", lastNode) + s.Equal("0", majorSets[0].GetVersions()[0]) // Ensure depth limiting works res, err = s.matchingEngine.GetWorkerBuildIdCompatability(s.handlerContext, &matchingservice.GetWorkerBuildIdCompatabilityRequest{ @@ -1997,9 +1996,12 @@ func (s *matchingEngineSuite) TestGetVersioningData() { }, }) s.NoError(err) - //s.NotNil(res.GetResponse().GetCurrentDefault()) - //s.Nil(res.GetResponse().GetCurrentDefault().GetPreviousIncompatible()) - //s.Nil(res.GetResponse().GetCompatibleLeaves()[0].GetPreviousCompatible()) + majorSets = res.GetResponse().GetMajorVersionSets() + curDefault = majorSets[len(majorSets)-1] + s.Equal("99", curDefault.GetVersions()[0]) + lastNode = curDefault.GetVersions()[len(curDefault.GetVersions())-1] + s.Equal("99.9", lastNode) + s.Equal(1, len(majorSets)) res, err = s.matchingEngine.GetWorkerBuildIdCompatability(s.handlerContext, &matchingservice.GetWorkerBuildIdCompatabilityRequest{ NamespaceId: namespaceID.String(), @@ -2010,23 +2012,8 @@ func (s *matchingEngineSuite) TestGetVersioningData() { }, }) s.NoError(err) - //s.NotNil(res.GetResponse().GetCurrentDefault()) - //lastNode = res.GetResponse().GetCurrentDefault() - //for { - // if lastNode.GetPreviousIncompatible() == nil { - // break - // } - // lastNode = lastNode.GetPreviousIncompatible() - //} - //s.Equal(mkVerId("95"), lastNode.GetVersion()) - //lastNode = res.GetResponse().GetCompatibleLeaves()[0] - //for { - // if lastNode.GetPreviousCompatible() == nil { - // break - // } - // lastNode = lastNode.GetPreviousCompatible() - //} - //s.Equal(mkVerId("99.5"), lastNode.GetVersion()) + majorSets = res.GetResponse().GetMajorVersionSets() + s.Equal("95", majorSets[0].GetVersions()[0]) } func (s *matchingEngineSuite) TestActivityQueueMetadataInvalidate() { diff --git a/service/matching/version_sets.go b/service/matching/version_sets.go index db8cadeaadc6..655423c59fec 100644 --- a/service/matching/version_sets.go +++ b/service/matching/version_sets.go @@ -35,7 +35,6 @@ import ( ) func ToBuildIdOrderingResponse(g *persistence.VersioningData, maxDepth int) *workflowservice.GetWorkerBuildIdCompatabilityResponse { - // TODO: Current default pointer not represented in response. Can either shuffle it to front or change API. return depthLimiter(g, maxDepth, false) } @@ -56,9 +55,11 @@ func HashVersioningData(data *persistence.VersioningData) []byte { func depthLimiter(g *persistence.VersioningData, maxDepth int, mutate bool) *workflowservice.GetWorkerBuildIdCompatabilityResponse { if maxDepth <= 0 || maxDepth >= len(g.GetVersionSets()) { - return &workflowservice.GetWorkerBuildIdCompatabilityResponse{MajorVersionSets: g.VersionSets} + return &workflowservice.GetWorkerBuildIdCompatabilityResponse{MajorVersionSets: g.GetVersionSets()} } - shortened := g.GetVersionSets()[maxDepth:] + sets := g.GetVersionSets() + startIndex := len(sets) - maxDepth + shortened := g.GetVersionSets()[startIndex:] if mutate { g.VersionSets = shortened } @@ -92,7 +93,7 @@ func depthLimiter(g *persistence.VersioningData, maxDepth int, mutate bool) *wor // Deletions are not permitted, as inserting new versions can accomplish the same goals with less complexity. However, // sets may be dropped when the number of sets limit is reached. They are dropped oldest first - the current default set // is never dropped, instead dropping the next oldest set. -func UpdateVersionsGraph(existingData *persistence.VersioningData, req *workflowservice.UpdateWorkerBuildIdCompatabilityRequest, maxSize int) error { +func UpdateVersionSets(existingData *persistence.VersioningData, req *workflowservice.UpdateWorkerBuildIdCompatabilityRequest, maxSize int) error { err := updateImpl(existingData, req) if err != nil { return err diff --git a/service/matching/version_sets_test.go b/service/matching/version_sets_test.go index b055cdf39e3b..164932fa54df 100644 --- a/service/matching/version_sets_test.go +++ b/service/matching/version_sets_test.go @@ -89,7 +89,7 @@ func TestNewDefaultUpdate(t *testing.T) { data := mkInitialData(2) req := mkNewDefReq("2") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -105,7 +105,7 @@ func TestNewDefaultGraphUpdateOfEmptyGraph(t *testing.T) { data := &persistencepb.VersioningData{} req := mkNewDefReq("1") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -117,7 +117,7 @@ func TestNewDefaultGraphUpdateCompatWithCurDefault(t *testing.T) { data := mkInitialData(2) req := mkNewCompatReq("1.1", "1", true) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -130,7 +130,7 @@ func TestNewDefaultGraphUpdateCompatWithNonDefaultSet(t *testing.T) { data := mkInitialData(2) req := mkNewCompatReq("0.1", "0", true) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -143,7 +143,7 @@ func TestNewCompatibleWithVerInOlderSet(t *testing.T) { data := mkInitialData(3) req := mkNewCompatReq("0.1", "0", false) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -159,11 +159,11 @@ func TestNewCompatibleWithNonDefaultGraphUpdate(t *testing.T) { data := mkInitialData(2) req := mkNewCompatReq("0.1", "0", false) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) req = mkNewCompatReq("0.2", "0.1", false) - err = UpdateVersionsGraph(data, req, 0) + err = UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[len(data.VersionSets)-1] @@ -174,7 +174,7 @@ func TestNewCompatibleWithNonDefaultGraphUpdate(t *testing.T) { // Ensure setting a compatible version which targets a non-leaf compat version ends up without a branch req = mkNewCompatReq("0.3", "0.1", false) - err = UpdateVersionsGraph(data, req, 0) + err = UpdateVersionSets(data, req, 0) assert.NoError(t, err) assert.Equal(t, "1", curd.Versions[0]) @@ -188,7 +188,7 @@ func TestCompatibleTargetsNotFound(t *testing.T) { data := mkInitialData(1) req := mkNewCompatReq("1.1", "1", false) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.Error(t, err) assert.IsType(t, &serviceerror.NotFound{}, err) } @@ -197,7 +197,7 @@ func TestMakeExistingSetDefault(t *testing.T) { data := mkInitialData(4) req := mkExistingDefault("2") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) assert.Equal(t, "0", data.VersionSets[0].Versions[0]) @@ -208,7 +208,7 @@ func TestMakeExistingSetDefault(t *testing.T) { // Add a compatible version to a set and then make that set the default via the compatible version req = mkNewCompatReq("1.1", "1", true) - err = UpdateVersionsGraph(data, req, 0) + err = UpdateVersionSets(data, req, 0) assert.NoError(t, err) assert.Equal(t, "0", data.VersionSets[0].Versions[0]) assert.Equal(t, "3", data.VersionSets[1].Versions[0]) @@ -220,11 +220,11 @@ func TestSayVersionIsCompatWithDifferentSetThanItsAlreadyCompatWithNotAllowed(t data := mkInitialData(3) req := mkNewCompatReq("0.1", "0", false) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) req = mkNewCompatReq("0.1", "1", false) - err = UpdateVersionsGraph(data, req, 0) + err = UpdateVersionSets(data, req, 0) assert.Error(t, err) assert.IsType(t, &serviceerror.InvalidArgument{}, err) } @@ -236,7 +236,7 @@ func TestLimitsMaxSize(t *testing.T) { for i := 0; i < 20; i++ { id := fmt.Sprintf("%d", i) req := mkNewDefReq(id) - err := UpdateVersionsGraph(data, req, maxSize) + err := UpdateVersionSets(data, req, maxSize) assert.NoError(t, err) } @@ -250,12 +250,12 @@ func TestPromoteWithinVersion(t *testing.T) { for i := 1; i <= 5; i++ { req := mkNewCompatReq(fmt.Sprintf("0.%d", i), "0", false) - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) } req := mkPromoteInSet("0.1") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.NoError(t, err) curd := data.VersionSets[0].Versions[len(data.VersionSets[0].Versions)-1] @@ -266,7 +266,16 @@ func TestAddAlreadyExtantVersionAsDefaultErrors(t *testing.T) { data := mkInitialData(3) req := mkNewDefReq("0") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) + assert.Error(t, err) + assert.IsType(t, &serviceerror.InvalidArgument{}, err) +} + +func TestAddAlreadyExtantVersionToAnotherSetErrors(t *testing.T) { + data := mkInitialData(3) + + req := mkNewCompatReq("0", "1", false) + err := UpdateVersionSets(data, req, 0) assert.Error(t, err) assert.IsType(t, &serviceerror.InvalidArgument{}, err) } @@ -275,7 +284,7 @@ func TestMakeSetDefaultTargetingNonexistentVersionErrors(t *testing.T) { data := mkInitialData(3) req := mkExistingDefault("crab boi") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.Error(t, err) assert.IsType(t, &serviceerror.NotFound{}, err) } @@ -284,7 +293,7 @@ func TestPromoteWithinSetTargetingNonexistentVersionErrors(t *testing.T) { data := mkInitialData(3) req := mkPromoteInSet("i'd rather be writing rust ;)") - err := UpdateVersionsGraph(data, req, 0) + err := UpdateVersionSets(data, req, 0) assert.Error(t, err) assert.IsType(t, &serviceerror.NotFound{}, err) }