diff --git a/client/history/client.go b/client/history/client.go index 75d75c22a93..eda742154e3 100644 --- a/client/history/client.go +++ b/client/history/client.go @@ -22,6 +22,7 @@ package history import ( "context" + "strconv" "sync" "time" @@ -1048,7 +1049,7 @@ func (c *clientImpl) getClientForDomainID(domainID string) (Client, error) { } func (c *clientImpl) getClientForShardID(shardID int) (Client, error) { - client, err := c.clients.GetClientForKey(string(shardID)) + client, err := c.clients.GetClientForKey(strconv.Itoa(shardID)) if err != nil { return nil, err } diff --git a/revive.toml b/revive.toml index 88115c31338..6857fd6b1b0 100644 --- a/revive.toml +++ b/revive.toml @@ -27,3 +27,8 @@ warningCode = 0 [rule.var-declaration] [rule.var-naming] +#### higher value stuff + +# string(int) is almost always a bug. +# go vet considers this a fatal error, but only in 1.15 or newer, and go.mod currently targets 1.13 +[rule.string-of-int] diff --git a/service/frontend/adminHandler.go b/service/frontend/adminHandler.go index 5515c91a608..f4168390410 100644 --- a/service/frontend/adminHandler.go +++ b/service/frontend/adminHandler.go @@ -248,7 +248,7 @@ func (adh *adminHandlerImpl) DescribeWorkflowExecution( } shardID := common.WorkflowIDToHistoryShard(request.Execution.WorkflowID, adh.numberOfHistoryShards) - shardIDstr := string(shardID) + shardIDstr := string(rune(shardID)) // originally `string(int_shard_id)`, but changing it will change the ring hashing shardIDForOutput := strconv.Itoa(shardID) historyHost, err := adh.GetMembershipMonitor().Lookup(common.HistoryServiceName, shardIDstr) diff --git a/service/history/shard/controller.go b/service/history/shard/controller.go index cb804717580..695aef36790 100644 --- a/service/history/shard/controller.go +++ b/service/history/shard/controller.go @@ -278,7 +278,7 @@ func (c *controller) getOrCreateHistoryShardItem(shardID int) (*historyShardsIte if c.isShuttingDown() || atomic.LoadInt32(&c.status) == common.DaemonStatusStopped { return nil, fmt.Errorf("controller for host '%v' shutting down", c.GetHostInfo().Identity()) } - info, err := c.GetHistoryServiceResolver().Lookup(string(shardID)) + info, err := c.GetHistoryServiceResolver().Lookup(fmt.Sprint(shardID)) if err != nil { return nil, err } @@ -378,7 +378,7 @@ func (c *controller) acquireShards() { if c.isShuttingDown() { return } - info, err := c.GetHistoryServiceResolver().Lookup(string(shardID)) + info, err := c.GetHistoryServiceResolver().Lookup(fmt.Sprint(shardID)) if err != nil { c.logger.Error("Error looking up host for shardID", tag.Error(err), tag.OperationFailed, tag.ShardID(shardID)) } else { diff --git a/service/history/shard/controller_test.go b/service/history/shard/controller_test.go index d7021f38dae..2d07fc1e3ab 100644 --- a/service/history/shard/controller_test.go +++ b/service/history/shard/controller_test.go @@ -112,7 +112,7 @@ func (s *controllerSuite) TestAcquireShardSuccess() { if hostID == 0 { myShards = append(myShards, shardID) s.mockHistoryEngine.EXPECT().Start().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2) s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1) s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return( &persistence.GetShardResponse{ @@ -160,7 +160,7 @@ func (s *controllerSuite) TestAcquireShardSuccess() { }).Return(nil).Once() } else { ownerHost := fmt.Sprintf("test-acquire-shard-host-%v", hostID) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1) } } @@ -195,7 +195,7 @@ func (s *controllerSuite) TestAcquireShardsConcurrently() { if hostID == 0 { myShards = append(myShards, shardID) s.mockHistoryEngine.EXPECT().Start().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2) s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1) s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return( &persistence.GetShardResponse{ @@ -243,7 +243,7 @@ func (s *controllerSuite) TestAcquireShardsConcurrently() { }).Return(nil).Once() } else { ownerHost := fmt.Sprintf("test-acquire-shard-host-%v", hostID) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1) } } @@ -263,12 +263,12 @@ func (s *controllerSuite) TestAcquireShardLookupFailure() { numShards := 2 s.config.NumberOfShards = numShards for shardID := 0; shardID < numShards; shardID++ { - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1) } s.shardController.acquireShards() for shardID := 0; shardID < numShards; shardID++ { - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1) s.Nil(s.shardController.GetEngineForShard(shardID)) } } @@ -285,7 +285,7 @@ func (s *controllerSuite) TestAcquireShardRenewSuccess() { for shardID := 0; shardID < numShards; shardID++ { s.mockHistoryEngine.EXPECT().Start().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2) s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1) s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return( &persistence.GetShardResponse{ @@ -339,7 +339,7 @@ func (s *controllerSuite) TestAcquireShardRenewSuccess() { s.shardController.acquireShards() for shardID := 0; shardID < numShards; shardID++ { - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(1) } s.shardController.acquireShards() @@ -360,7 +360,7 @@ func (s *controllerSuite) TestAcquireShardRenewLookupFailed() { for shardID := 0; shardID < numShards; shardID++ { s.mockHistoryEngine.EXPECT().Start().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2) s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1) s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return( &persistence.GetShardResponse{ @@ -414,7 +414,7 @@ func (s *controllerSuite) TestAcquireShardRenewLookupFailed() { s.shardController.acquireShards() for shardID := 0; shardID < numShards; shardID++ { - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1) } s.shardController.acquireShards() @@ -461,7 +461,7 @@ func (s *controllerSuite) TestHistoryEngineClosed() { for shardID := 0; shardID < 2; shardID++ { mockEngine := historyEngines[shardID] mockEngine.EXPECT().Stop().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(differentHostInfo, nil).AnyTimes() + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(differentHostInfo, nil).AnyTimes() s.shardController.shardClosedCallback(shardID, nil) } @@ -506,7 +506,7 @@ func (s *controllerSuite) TestHistoryEngineClosed() { for shardID := 2; shardID < numShards; shardID++ { mockEngine := historyEngines[shardID] mockEngine.EXPECT().Stop().Return().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).AnyTimes() + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).AnyTimes() } s.shardController.Stop() } @@ -553,7 +553,7 @@ func (s *controllerSuite) TestShardControllerClosed() { for shardID := 0; shardID < numShards; shardID++ { mockEngine := historyEngines[shardID] mockEngine.EXPECT().Stop().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).AnyTimes() + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).AnyTimes() } s.shardController.Stop() workerWG.Wait() @@ -570,7 +570,7 @@ func (s *controllerSuite) setupMocksForAcquireShard(shardID int, mockEngine *eng // s.mockResource.ExecutionMgr.On("Close").Return() mockEngine.EXPECT().Start().Times(1) - s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2) + s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2) s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(mockEngine).Times(1) s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return( &persistence.GetShardResponse{ diff --git a/service/history/task/transfer_active_task_executor_test.go b/service/history/task/transfer_active_task_executor_test.go index b8aa2569e18..d1cd0a8c8b2 100644 --- a/service/history/task/transfer_active_task_executor_test.go +++ b/service/history/task/transfer_active_task_executor_test.go @@ -23,6 +23,7 @@ package task import ( "context" "math/rand" + "strconv" "testing" "time" @@ -942,7 +943,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has decisions = append(decisions, &types.Decision{ DecisionType: &dt, StartChildWorkflowExecutionDecisionAttributes: &types.StartChildWorkflowExecutionDecisionAttributes{ - WorkflowID: "child workflow" + string(i), + WorkflowID: "child workflow" + strconv.Itoa(i), WorkflowType: &types.WorkflowType{ Name: "child workflow type", }, @@ -961,7 +962,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has for i := 0; i < 10; i++ { _, _, err = mutableState.AddStartChildWorkflowExecutionInitiatedEvent(event.GetEventID(), uuid.New(), &types.StartChildWorkflowExecutionDecisionAttributes{ - WorkflowID: "child workflow" + string(i), + WorkflowID: "child workflow" + strconv.Itoa(i), WorkflowType: &types.WorkflowType{ Name: "child workflow type", }, @@ -1039,7 +1040,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has decisions = append(decisions, &types.Decision{ DecisionType: &dt, StartChildWorkflowExecutionDecisionAttributes: &types.StartChildWorkflowExecutionDecisionAttributes{ - WorkflowID: "child workflow" + string(i), + WorkflowID: "child workflow" + strconv.Itoa(i), WorkflowType: &types.WorkflowType{ Name: "child workflow type", }, @@ -1058,7 +1059,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has for i := 0; i < 10; i++ { _, _, err = mutableState.AddStartChildWorkflowExecutionInitiatedEvent(event.GetEventID(), uuid.New(), &types.StartChildWorkflowExecutionDecisionAttributes{ - WorkflowID: "child workflow" + string(i), + WorkflowID: "child workflow" + strconv.Itoa(i), WorkflowType: &types.WorkflowType{ Name: "child workflow type", },