From 0683185e043989db90a083b777016bd88278f4bc Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Wed, 3 May 2023 12:29:09 -0700 Subject: [PATCH 1/4] Fix task executable attempt reset --- service/history/queues/executable.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/service/history/queues/executable.go b/service/history/queues/executable.go index 2b2e7d29e59..5f9e734d82c 100644 --- a/service/history/queues/executable.go +++ b/service/history/queues/executable.go @@ -212,10 +212,9 @@ func (e *executableImpl) Execute() (retErr error) { e.taggedMetricsHandler = e.metricsHandler.WithTags(metricsTags...) if isActive != e.lastActiveness { - // namespace did a failover, reset task attempt - e.Lock() - e.attempt = 0 - e.Unlock() + // namespace did a failover, + // reset task attempt since the execution logic used will change + e.resetAttempt() } e.lastActiveness = isActive @@ -535,6 +534,13 @@ func (e *executableImpl) updatePriority() { } } +func (e *executableImpl) resetAttempt() { + e.Lock() + defer e.Unlock() + + e.attempt = 1 +} + func (e *executableImpl) estimateTaskMetricTag() []metrics.Tag { namespaceTag := metrics.NamespaceUnknownTag() isActive := true From ab1e90ab099e885c2c7031602e3d86442008b9a4 Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Wed, 3 May 2023 12:43:46 -0700 Subject: [PATCH 2/4] add tests --- service/history/queues/executable_test.go | 24 +++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/service/history/queues/executable_test.go b/service/history/queues/executable_test.go index b5e71fd3bd6..a1b22daafbd 100644 --- a/service/history/queues/executable_test.go +++ b/service/history/queues/executable_test.go @@ -149,6 +149,30 @@ func (s *executableSuite) TestExecute_CallerInfo() { s.NoError(executable.Execute()) } +func (s *executableSuite) TestExecuteHandleErr_ResetAttempt() { + executable := s.newTestExecutable() + s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).DoAndReturn( + func(ctx context.Context, _ Executable) ([]metrics.Tag, bool, error) { + s.Equal(headers.CallerTypeBackground, headers.GetCallerInfo(ctx).CallerType) + return nil, true, errors.New("some random error") + }, + ) + err := executable.Execute() + s.Error(err) + executable.HandleErr(err) + s.Equal(2, executable.Attempt()) + + s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).DoAndReturn( + func(ctx context.Context, _ Executable) ([]metrics.Tag, bool, error) { + s.Equal(headers.CallerTypeBackground, headers.GetCallerInfo(ctx).CallerType) + // isActive changed to false, should reset attempt + return nil, false, nil + }, + ) + s.NoError(executable.Execute()) + s.Equal(1, executable.Attempt()) +} + func (s *executableSuite) TestExecuteHandleErr_Corrupted() { executable := s.newTestExecutable() From f40adaf4475ebf71f0702ef0b5c4d520bdd117f1 Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Wed, 3 May 2023 12:51:30 -0700 Subject: [PATCH 3/4] simplify tests --- service/history/queues/executable_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/service/history/queues/executable_test.go b/service/history/queues/executable_test.go index a1b22daafbd..fe22490d79f 100644 --- a/service/history/queues/executable_test.go +++ b/service/history/queues/executable_test.go @@ -151,24 +151,14 @@ func (s *executableSuite) TestExecute_CallerInfo() { func (s *executableSuite) TestExecuteHandleErr_ResetAttempt() { executable := s.newTestExecutable() - s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).DoAndReturn( - func(ctx context.Context, _ Executable) ([]metrics.Tag, bool, error) { - s.Equal(headers.CallerTypeBackground, headers.GetCallerInfo(ctx).CallerType) - return nil, true, errors.New("some random error") - }, - ) + s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).Return(nil, true, errors.New("some random error")) err := executable.Execute() s.Error(err) executable.HandleErr(err) s.Equal(2, executable.Attempt()) - s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).DoAndReturn( - func(ctx context.Context, _ Executable) ([]metrics.Tag, bool, error) { - s.Equal(headers.CallerTypeBackground, headers.GetCallerInfo(ctx).CallerType) - // isActive changed to false, should reset attempt - return nil, false, nil - }, - ) + // isActive changed to false, should reset attempt + s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).Return(nil, false, nil) s.NoError(executable.Execute()) s.Equal(1, executable.Attempt()) } From fdb6758d63d93efa28468bc6a00479993fa18795 Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Wed, 3 May 2023 13:07:14 -0700 Subject: [PATCH 4/4] fix test --- service/history/queues/executable_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/history/queues/executable_test.go b/service/history/queues/executable_test.go index fe22490d79f..df0f18b4f68 100644 --- a/service/history/queues/executable_test.go +++ b/service/history/queues/executable_test.go @@ -154,7 +154,7 @@ func (s *executableSuite) TestExecuteHandleErr_ResetAttempt() { s.mockExecutor.EXPECT().Execute(gomock.Any(), executable).Return(nil, true, errors.New("some random error")) err := executable.Execute() s.Error(err) - executable.HandleErr(err) + s.Error(executable.HandleErr(err)) s.Equal(2, executable.Attempt()) // isActive changed to false, should reset attempt