From 91e05cebf4d49d6d83a5a3dcc80f3ff9654644d1 Mon Sep 17 00:00:00 2001 From: Qiang Zhao Date: Sat, 25 Jan 2025 09:11:23 +0800 Subject: [PATCH 1/2] feat: make addFollower idempotent to avoid FollowerAlreadyPresent --- common/error_codes.go | 2 -- coordinator/impl/shard_controller.go | 2 +- server/leader_controller.go | 2 +- server/leader_controller_test.go | 8 ++++---- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/common/error_codes.go b/common/error_codes.go index b4205c3f..6e8c9fcc 100644 --- a/common/error_codes.go +++ b/common/error_codes.go @@ -32,7 +32,6 @@ const ( CodeInvalidSessionTimeout codes.Code = 109 CodeNamespaceNotFound codes.Code = 110 CodeNotificationsNotEnabled codes.Code = 111 - CodeFollowerAlreadyPresent codes.Code = 112 CodeFollowerAlreadyFenced codes.Code = 113 ) @@ -49,6 +48,5 @@ var ( ErrorInvalidSessionTimeout = status.Error(CodeInvalidSessionTimeout, "oxia: invalid session timeout") ErrorNamespaceNotFound = status.Error(CodeNamespaceNotFound, "oxia: namespace not found") ErrorNotificationsNotEnabled = status.Error(CodeNotificationsNotEnabled, "oxia: notifications not enabled on namespace") - ErrorFollowerAlreadyPresent = status.Error(CodeFollowerAlreadyPresent, "oxia: follower is already present") ErrorFollowerAlreadyFenced = status.Error(CodeFollowerAlreadyFenced, "oxia: follower is already fenced") ) diff --git a/coordinator/impl/shard_controller.go b/coordinator/impl/shard_controller.go index 3e919079..8f7258aa 100644 --- a/coordinator/impl/shard_controller.go +++ b/coordinator/impl/shard_controller.go @@ -512,7 +512,7 @@ func (s *shardController) internalNewTermAndAddFollower(ctx context.Context, nod if err = s.addFollower(*s.shardMetadata.Leader, node.Internal, &proto.EntryId{ Term: fr.Term, Offset: fr.Offset, - }); err != nil && status.Code(err) != common.CodeFollowerAlreadyPresent { + }); err != nil { res <- err return } diff --git a/server/leader_controller.go b/server/leader_controller.go index 09c1f9cb..19bbc445 100644 --- a/server/leader_controller.go +++ b/server/leader_controller.go @@ -378,7 +378,7 @@ func (lc *leaderController) AddFollower(req *proto.AddFollowerRequest) (*proto.A } if _, followerAlreadyPresent := lc.followers[req.FollowerName]; followerAlreadyPresent { - return nil, errors.Wrapf(common.ErrorFollowerAlreadyPresent, "follower: %s", req.FollowerName) + return &proto.AddFollowerResponse{}, nil } if len(lc.followers) == int(lc.replicationFactor)-1 { diff --git a/server/leader_controller_test.go b/server/leader_controller_test.go index f86a9946..e434158e 100644 --- a/server/leader_controller_test.go +++ b/server/leader_controller_test.go @@ -589,8 +589,8 @@ func TestLeaderController_AddFollowerRepeated(t *testing.T) { FollowerName: "f1", FollowerHeadEntryId: InvalidEntryId, }) - assert.Nil(t, afRes) - assert.Error(t, err) + assert.Nil(t, err) + assert.Equal(t, &proto.AddFollowerResponse{}, afRes) _, err = lc.AddFollower(&proto.AddFollowerRequest{ Shard: shard, @@ -598,8 +598,8 @@ func TestLeaderController_AddFollowerRepeated(t *testing.T) { FollowerName: "f1", FollowerHeadEntryId: InvalidEntryId, }) - assert.Error(t, err) - assert.Equal(t, common.CodeFollowerAlreadyPresent, status.Code(err)) + assert.Nil(t, err) + assert.Equal(t, &proto.AddFollowerResponse{}, afRes) assert.NoError(t, lc.Close()) assert.NoError(t, kvFactory.Close()) From a1ad1a86c790113013b37819aa3c35d30aa8ffce Mon Sep 17 00:00:00 2001 From: Qiang Zhao Date: Sat, 25 Jan 2025 09:41:29 +0800 Subject: [PATCH 2/2] fix test --- server/leader_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/leader_controller_test.go b/server/leader_controller_test.go index e434158e..bdc73fb3 100644 --- a/server/leader_controller_test.go +++ b/server/leader_controller_test.go @@ -526,8 +526,8 @@ func TestLeaderController_AddFollower(t *testing.T) { FollowerName: "f1", FollowerHeadEntryId: InvalidEntryId, }) - assert.Nil(t, afRes) - assert.Error(t, err) + assert.Equal(t, &proto.AddFollowerResponse{}, afRes) + assert.Nil(t, err) _, err = lc.AddFollower(&proto.AddFollowerRequest{ Shard: shard,