Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: changes to storage-related transaction handling within AutoOps packages #1365

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

refactor: changes to storage-related transaction handling within AutoOps packages #1365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2387740
refactor: fefactor of AutoOps Storage transaction processing
kakcy Dec 4, 2024
abdd640
refactor: refactor of ProgressiveRollout Storage transaction processing
kakcy Dec 4, 2024
a0bd58e
test: fix unit test for AutoOps package
kakcy Dec 5, 2024
6e1ae57
refactor: fix lint
kakcy Dec 5, 2024
96ade3f
refactor: changed transaction handling in other packages related to P…
kakcy Dec 6, 2024
f9f281d
refactor: change RunInTransactionV2
kakcy Dec 24, 2024
cfa26ee
test: fix test
kakcy Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 27 additions & 108 deletions pkg/autoops/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,33 +182,17 @@ func (s *AutoOpsService) CreateAutoOpsRule(
return nil, dt.Err()
}
}
tx, err := s.mysqlClient.BeginTx(ctx)
if err != nil {
s.logger.Error(
"Failed to begin transaction",
log.FieldsFromImcomingContext(ctx).AddFields(
zap.Error(err),
)...,
)
dt, err := statusInternal.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InternalServerError),
})
if err != nil {
return nil, statusInternal.Err()
}
return nil, dt.Err()
}
err = s.mysqlClient.RunInTransaction(ctx, tx, func() error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(tx)

err = s.mysqlClient.RunInTransactionV2(ctx, func(contextWithTx context.Context, _ mysql.Transaction) error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(s.mysqlClient)
handler, err := command.NewAutoOpsCommandHandler(editor, autoOpsRule, s.publisher, req.EnvironmentId)
if err != nil {
return err
}
if err := handler.Handle(ctx, req.Command); err != nil {
return err
}
return autoOpsRuleStorage.CreateAutoOpsRule(ctx, autoOpsRule, req.EnvironmentId)
return autoOpsRuleStorage.CreateAutoOpsRule(contextWithTx, autoOpsRule, req.EnvironmentId)
})
if err != nil {
if err == v2as.ErrAutoOpsRuleAlreadyExists {
Expand Down Expand Up @@ -473,27 +457,10 @@ func (s *AutoOpsService) StopAutoOpsRule(
if err := validateStopAutoOpsRuleRequest(req, localizer); err != nil {
return nil, err
}
tx, err := s.mysqlClient.BeginTx(ctx)
if err != nil {
s.logger.Error(
"Failed to begin transaction",
log.FieldsFromImcomingContext(ctx).AddFields(
zap.Error(err),
)...,
)
dt, err := statusInternal.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InternalServerError),
})
if err != nil {
return nil, statusInternal.Err()
}
return nil, dt.Err()
}

err = s.mysqlClient.RunInTransaction(ctx, tx, func() error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(tx)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(ctx, req.Id, req.EnvironmentId)
err = s.mysqlClient.RunInTransactionV2(ctx, func(contextWithTx context.Context, _ mysql.Transaction) error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(s.mysqlClient)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(contextWithTx, req.Id, req.EnvironmentId)
if err != nil {
return err
}
Expand All @@ -514,7 +481,7 @@ func (s *AutoOpsService) StopAutoOpsRule(
if err := handler.Handle(ctx, req.Command); err != nil {
return err
}
return autoOpsRuleStorage.UpdateAutoOpsRule(ctx, autoOpsRule, req.EnvironmentId)
return autoOpsRuleStorage.UpdateAutoOpsRule(contextWithTx, autoOpsRule, req.EnvironmentId)
})

if err != nil {
Expand Down Expand Up @@ -561,26 +528,10 @@ func (s *AutoOpsService) DeleteAutoOpsRule(
if err := validateDeleteAutoOpsRuleRequest(req, localizer); err != nil {
return nil, err
}
tx, err := s.mysqlClient.BeginTx(ctx)
if err != nil {
s.logger.Error(
"Failed to begin transaction",
log.FieldsFromImcomingContext(ctx).AddFields(
zap.Error(err),
)...,
)
dt, err := statusInternal.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InternalServerError),
})
if err != nil {
return nil, statusInternal.Err()
}
return nil, dt.Err()
}
err = s.mysqlClient.RunInTransaction(ctx, tx, func() error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(tx)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(ctx, req.Id, req.EnvironmentId)

err = s.mysqlClient.RunInTransactionV2(ctx, func(contextWithTx context.Context, _ mysql.Transaction) error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(s.mysqlClient)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(contextWithTx, req.Id, req.EnvironmentId)
if err != nil {
return err
}
Expand All @@ -591,7 +542,7 @@ func (s *AutoOpsService) DeleteAutoOpsRule(
if err := handler.Handle(ctx, req.Command); err != nil {
return err
}
return autoOpsRuleStorage.UpdateAutoOpsRule(ctx, autoOpsRule, req.EnvironmentId)
return autoOpsRuleStorage.UpdateAutoOpsRule(contextWithTx, autoOpsRule, req.EnvironmentId)
})
if err != nil {
if err == v2as.ErrAutoOpsRuleNotFound || err == v2as.ErrAutoOpsRuleUnexpectedAffectedRows {
Expand Down Expand Up @@ -696,26 +647,10 @@ func (s *AutoOpsService) UpdateAutoOpsRule(
}
}
commands := s.createUpdateAutoOpsRuleCommands(req)
tx, err := s.mysqlClient.BeginTx(ctx)
if err != nil {
s.logger.Error(
"Failed to begin transaction",
log.FieldsFromImcomingContext(ctx).AddFields(
zap.Error(err),
)...,
)
dt, err := statusInternal.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InternalServerError),
})
if err != nil {
return nil, statusInternal.Err()
}
return nil, dt.Err()
}
err = s.mysqlClient.RunInTransaction(ctx, tx, func() error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(tx)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(ctx, req.Id, req.EnvironmentId)

err = s.mysqlClient.RunInTransactionV2(ctx, func(contextWithTx context.Context, _ mysql.Transaction) error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(s.mysqlClient)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(contextWithTx, req.Id, req.EnvironmentId)
if err != nil {
return err
}
Expand Down Expand Up @@ -817,7 +752,7 @@ func (s *AutoOpsService) UpdateAutoOpsRule(
return err
}
}
return autoOpsRuleStorage.UpdateAutoOpsRule(ctx, autoOpsRule, req.EnvironmentId)
return autoOpsRuleStorage.UpdateAutoOpsRule(contextWithTx, autoOpsRule, req.EnvironmentId)
})
if err != nil {
if err == v2as.ErrAutoOpsRuleNotFound || err == v2as.ErrAutoOpsRuleUnexpectedAffectedRows {
Expand Down Expand Up @@ -1187,26 +1122,10 @@ func (s *AutoOpsService) ExecuteAutoOps(
if triggered {
return &autoopsproto.ExecuteAutoOpsResponse{AlreadyTriggered: true}, nil
}
tx, err := s.mysqlClient.BeginTx(ctx)
if err != nil {
s.logger.Error(
"Failed to begin transaction",
log.FieldsFromImcomingContext(ctx).AddFields(
zap.Error(err),
)...,
)
dt, err := statusInternal.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InternalServerError),
})
if err != nil {
return nil, statusInternal.Err()
}
return nil, dt.Err()
}
err = s.mysqlClient.RunInTransaction(ctx, tx, func() error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(tx)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(ctx, req.Id, req.EnvironmentId)

err = s.mysqlClient.RunInTransactionV2(ctx, func(contextWithTx context.Context, tx mysql.Transaction) error {
autoOpsRuleStorage := v2as.NewAutoOpsRuleStorage(s.mysqlClient)
autoOpsRule, err := autoOpsRuleStorage.GetAutoOpsRule(contextWithTx, req.Id, req.EnvironmentId)
if err != nil {
return err
}
Expand All @@ -1231,15 +1150,15 @@ func (s *AutoOpsService) ExecuteAutoOps(
}

ftStorage := ftstorage.NewFeatureStorage(tx)
feature, err := ftStorage.GetFeature(ctx, autoOpsRule.FeatureId, req.EnvironmentId)
feature, err := ftStorage.GetFeature(contextWithTx, autoOpsRule.FeatureId, req.EnvironmentId)
if err != nil {
return err
}
prStorage := v2as.NewProgressiveRolloutStorage(tx)
prStorage := v2as.NewProgressiveRolloutStorage(s.mysqlClient)
// Stop the running progressive rollout if the operation type is disable
if executeClause.ActionType == autoopsproto.ActionType_DISABLE {
if err := s.stopProgressiveRollout(
ctx,
contextWithTx,
req.EnvironmentId,
autoOpsRule,
prStorage,
Expand All @@ -1249,7 +1168,7 @@ func (s *AutoOpsService) ExecuteAutoOps(
}
}
if err := executeAutoOpsRuleOperation(
ctx,
contextWithTx,
ftStorage,
req.EnvironmentId,
executeClause.ActionType,
Expand Down Expand Up @@ -1280,7 +1199,7 @@ func (s *AutoOpsService) ExecuteAutoOps(
return err
}

if err = autoOpsRuleStorage.UpdateAutoOpsRule(ctx, autoOpsRule, req.EnvironmentId); err != nil {
if err = autoOpsRuleStorage.UpdateAutoOpsRule(contextWithTx, autoOpsRule, req.EnvironmentId); err != nil {
if err == v2as.ErrAutoOpsRuleUnexpectedAffectedRows {
s.logger.Warn(
"No rows were affected",
Expand Down
73 changes: 48 additions & 25 deletions pkg/autoops/api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,8 @@ func TestCreateAutoOpsRuleMySQL(t *testing.T) {
s.experimentClient.(*experimentclientmock.MockClient).EXPECT().GetGoal(
gomock.Any(), gomock.Any(),
).Return(&experimentproto.GetGoalResponse{}, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.CreateAutoOpsRuleRequest{
Expand All @@ -393,9 +392,8 @@ func TestCreateAutoOpsRuleMySQL(t *testing.T) {
{
desc: "success schedule",
setup: func(s *AutoOpsService) {
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.CreateAutoOpsRuleRequest{
Expand Down Expand Up @@ -590,9 +588,8 @@ func TestUpdateAutoOpsRuleMySQL(t *testing.T) {
{
desc: "success",
setup: func(s *AutoOpsService) {
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.UpdateAutoOpsRuleRequest{
Expand Down Expand Up @@ -673,9 +670,8 @@ func TestStopAutoOpsRuleMySQL(t *testing.T) {
{
desc: "success",
setup: func(s *AutoOpsService) {
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.StopAutoOpsRuleRequest{
Expand Down Expand Up @@ -738,9 +734,8 @@ func TestDeleteAutoOpsRuleMySQL(t *testing.T) {
{
desc: "success",
setup: func(s *AutoOpsService) {
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.DeleteAutoOpsRuleRequest{
Expand Down Expand Up @@ -801,7 +796,11 @@ func TestGetAutoOpsRuleMySQL(t *testing.T) {
setup: func(s *AutoOpsService) {
row := mysqlmock.NewMockRow(mockController)
row.EXPECT().Scan(gomock.Any()).Return(mysql.ErrNoRows)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryRowContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryRowContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(row)
},
Expand All @@ -821,7 +820,11 @@ func TestGetAutoOpsRuleMySQL(t *testing.T) {
setup: func(s *AutoOpsService) {
row := mysqlmock.NewMockRow(mockController)
row.EXPECT().Scan(gomock.Any()).Return(nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryRowContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryRowContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(row)
},
Expand All @@ -834,7 +837,11 @@ func TestGetAutoOpsRuleMySQL(t *testing.T) {
setup: func(s *AutoOpsService) {
row := mysqlmock.NewMockRow(mockController)
row.EXPECT().Scan(gomock.Any()).Return(nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryRowContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryRowContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(row)
},
Expand Down Expand Up @@ -888,7 +895,11 @@ func TestListAutoOpsRulesMySQL(t *testing.T) {
rows.EXPECT().Close().Return(nil)
rows.EXPECT().Next().Return(false)
rows.EXPECT().Err().Return(nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(rows, nil)
},
Expand All @@ -910,7 +921,12 @@ func TestListAutoOpsRulesMySQL(t *testing.T) {
rows.EXPECT().Close().Return(nil)
rows.EXPECT().Next().Return(false)
rows.EXPECT().Err().Return(nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)

qe.EXPECT().QueryContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(rows, nil)
},
Expand Down Expand Up @@ -1058,7 +1074,11 @@ func TestExecuteAutoOpsRuleMySQL(t *testing.T) {
setup: func(s *AutoOpsService) {
row := mysqlmock.NewMockRow(mockController)
row.EXPECT().Scan(gomock.Any()).Return(mysql.ErrNoRows)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryRowContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryRowContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(row)
},
Expand All @@ -1076,12 +1096,15 @@ func TestExecuteAutoOpsRuleMySQL(t *testing.T) {
setup: func(s *AutoOpsService) {
row := mysqlmock.NewMockRow(mockController)
row.EXPECT().Scan(gomock.Any()).Return(nil)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().QueryRowContext(
qe := mysqlmock.NewMockQueryExecer(mockController)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().Qe(
gomock.Any(),
).Return(qe)
qe.EXPECT().QueryRowContext(
gomock.Any(), gomock.Any(), gomock.Any(),
).Return(row)
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().BeginTx(gomock.Any()).Return(nil, nil).AnyTimes()
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransaction(
gomock.Any(), gomock.Any(), gomock.Any(),
s.mysqlClient.(*mysqlmock.MockClient).EXPECT().RunInTransactionV2(
gomock.Any(), gomock.Any(),
).Return(nil)
},
req: &autoopsproto.ExecuteAutoOpsRequest{
Expand Down
Loading
Loading