From fe9c3ab96aa3ea5436da104f4808760d4dc6abdf Mon Sep 17 00:00:00 2001 From: Zijian Date: Wed, 15 Feb 2023 16:11:52 -0800 Subject: [PATCH] Unload taskListManager by instance, not taskListID (#5101) --- service/matching/matchingEngine.go | 40 +++++++++++--------- service/matching/matchingEngine_test.go | 49 +++++++++++++++++++++---- service/matching/taskListManager.go | 7 +++- 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/service/matching/matchingEngine.go b/service/matching/matchingEngine.go index 0bd12cfb78f..57af8d75fed 100644 --- a/service/matching/matchingEngine.go +++ b/service/matching/matchingEngine.go @@ -1,5 +1,7 @@ -// Copyright (c) 2017-2020 Uber Technologies, Inc. -// +// Copyright (c) 2017-2020 Uber Technologies Inc. + +// Portions of the Software are attributed to Copyright (c) 2020 Temporal Technologies Inc. + // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal // in the Software without restriction, including without limitation the rights @@ -7,16 +9,16 @@ // copies of the Software, and to permit persons to whom the Software is // furnished to do so, subject to the following conditions: // -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. package matching @@ -253,11 +255,14 @@ func (e *matchingEngineImpl) updateTaskList(taskList *taskListID, mgr taskListMa e.taskLists[*taskList] = mgr } -func (e *matchingEngineImpl) removeTaskListManager(id *taskListID) { +func (e *matchingEngineImpl) removeTaskListManager(tlMgr taskListManager) { + id := tlMgr.TaskListID() e.taskListsLock.Lock() - defer e.taskListsLock.Unlock() - - delete(e.taskLists, *id) + currentTlMgr, ok := e.taskLists[*id] + if ok && tlMgr == currentTlMgr { + delete(e.taskLists, *id) + } + e.taskListsLock.Unlock() e.metricsClient.Scope(metrics.MatchingTaskListMgrScope).UpdateGauge( metrics.TaskListManagersGauge, float64(len(e.taskLists)), @@ -836,16 +841,17 @@ func (e *matchingEngineImpl) getTask( return tlMgr.GetTask(ctx, maxDispatchPerSecond) } -func (e *matchingEngineImpl) unloadTaskList(id *taskListID) { +func (e *matchingEngineImpl) unloadTaskList(tlMgr taskListManager) { + id := tlMgr.TaskListID() e.taskListsLock.Lock() - tlMgr, ok := e.taskLists[*id] - if ok { - delete(e.taskLists, *id) + currentTlMgr, ok := e.taskLists[*id] + if !ok || tlMgr != currentTlMgr { + e.taskListsLock.Unlock() + return } + delete(e.taskLists, *id) e.taskListsLock.Unlock() - if ok { - tlMgr.Stop() - } + tlMgr.Stop() } // Populate the decision task response based on context and scheduled/started events. diff --git a/service/matching/matchingEngine_test.go b/service/matching/matchingEngine_test.go index 6c19008a996..a43013ddd7f 100644 --- a/service/matching/matchingEngine_test.go +++ b/service/matching/matchingEngine_test.go @@ -1,5 +1,7 @@ -// Copyright (c) 2017 Uber Technologies, Inc. -// +// Copyright (c) 2017-2020 Uber Technologies Inc. + +// Portions of the Software are attributed to Copyright (c) 2020 Temporal Technologies Inc. + // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal // in the Software without restriction, including without limitation the rights @@ -7,16 +9,16 @@ // copies of the Software, and to permit persons to whom the Software is // furnished to do so, subject to the following conditions: // -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. package matching @@ -175,6 +177,39 @@ func (s *matchingEngineSuite) TestPollForDecisionTasksEmptyResultWithShortContex s.PollForTasksEmptyResultTest(callContext, persistence.TaskListTypeDecision) } +func (s *matchingEngineSuite) TestOnlyUnloadMatchingInstance() { + taskListID := newTestTaskListID( + uuid.New(), + "makeToast", + persistence.TaskListTypeActivity) + tlKind := types.TaskListKindNormal + tlm, err := s.matchingEngine.getTaskListManager(taskListID, &tlKind) + s.Require().NoError(err) + + tlm2, err := newTaskListManager( + s.matchingEngine, + taskListID, // same taskListID as above + &tlKind, + s.matchingEngine.config) + s.Require().NoError(err) + + // try to unload a different tlm instance with the same taskListID + s.matchingEngine.unloadTaskList(tlm2) + + got, err := s.matchingEngine.getTaskListManager(taskListID, &tlKind) + s.Require().NoError(err) + s.Require().Same(tlm, got, + "Unload call with non-matching taskListManager should not cause unload") + + // this time unload the right tlm + s.matchingEngine.unloadTaskList(tlm) + + got, err = s.matchingEngine.getTaskListManager(taskListID, &tlKind) + s.Require().NoError(err) + s.Require().NotSame(tlm, got, + "Unload call with matching incarnation should have caused unload") +} + func (s *matchingEngineSuite) TestPollForDecisionTasks() { s.PollForDecisionTasksResultTest() } @@ -1496,7 +1531,7 @@ func (s *matchingEngineSuite) TestTaskListManagerGetTaskBatch() { s.True(0 < len(tasks) && len(tasks) <= rangeSize) s.True(isReadBatchDone) - tlMgr.engine.removeTaskListManager(tlMgr.taskListID) + tlMgr.engine.removeTaskListManager(tlMgr) } func (s *matchingEngineSuite) TestTaskListManagerGetTaskBatch_ReadBatchDone() { diff --git a/service/matching/taskListManager.go b/service/matching/taskListManager.go index 19b6982de48..e69bc6118f6 100644 --- a/service/matching/taskListManager.go +++ b/service/matching/taskListManager.go @@ -83,6 +83,7 @@ type ( DescribeTaskList(includeTaskListStatus bool) *types.DescribeTaskListResponse String() string GetTaskListKind() types.TaskListKind + TaskListID() *taskListID } // Single task list in memory state @@ -204,7 +205,7 @@ func (c *taskListManagerImpl) Stop() { close(c.shutdownCh) c.taskWriter.Stop() c.taskReader.Stop() - c.engine.removeTaskListManager(c.taskListID) + c.engine.removeTaskListManager(c) c.logger.Info("Task list manager state changed", tag.LifeCycleStopped) } @@ -428,6 +429,10 @@ func (c *taskListManagerImpl) GetTaskListKind() types.TaskListKind { return c.taskListKind } +func (c *taskListManagerImpl) TaskListID() *taskListID { + return c.taskListID +} + // completeTask marks a task as processed. Only tasks created by taskReader (i.e. backlog from db) reach // here. As part of completion: // - task is deleted from the database when err is nil