address comment

Signed-off-by: husharp <[email protected]>
HuSharp · Jul 18, 2024 · 39c9cf2 · 39c9cf2
1 parent cc99e73
commit 39c9cf2
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 36 deletions.
diff --git a/pkg/member/member.go b/pkg/member/member.go
@@ -43,11 +43,10 @@ const (
 	// The timeout to wait transfer etcd leader to complete.
 	moveLeaderTimeout          = 5 * time.Second
 	dcLocationConfigEtcdPrefix = "dc-location"
+	// If the campaign times is more than this value in `campaignTimesRecordTimeout`, the PD will resign and campaign again.
+	campaignLeaderFrequencyTimes = 3
 )
 
-// If the campaign times is more than this value in `campaignTimesRecordTimeout`, the PD will resign and campaign again.
-var campaignLeaderFrequencyTimes = 3
-
 // EmbeddedEtcdMember is used for the election related logic. It implements Member interface.
 type EmbeddedEtcdMember struct {
 	leadership *election.Leadership
@@ -188,7 +187,13 @@ func (m *EmbeddedEtcdMember) CampaignLeader(ctx context.Context, leaseTimeout in
 		failpoint.Return(m.leadership.Campaign(leaseTimeout, m.MemberValue()))
 	})
 
-	if m.leadership.GetCampaignTimesNum() > campaignLeaderFrequencyTimes {
+	checkTimes := campaignLeaderFrequencyTimes
+	failpoint.Inject("changeFrequencyTimes", func(val failpoint.Value) {
+		if v, ok := val.(int); ok {
+			checkTimes = v
+		}
+	})
+	if m.leadership.GetCampaignTimesNum() > checkTimes {
 		if err := m.ResignEtcdLeader(ctx, m.Name(), ""); err != nil {
 			return err
 		}
@@ -555,12 +560,3 @@ func (m *EmbeddedEtcdMember) SetMemberGitHash(id uint64, gitHash string) error {
 func (m *EmbeddedEtcdMember) Close() {
 	m.Etcd().Close()
 }
-
-// ChangeFrequencyTimes changes the frequency check times of campaign leader.
-// ONLY used for test to make the test more stable.
-// PLEASE flash back this value after using.
-func ChangeFrequencyTimes(times int) int {
-	before := campaignLeaderFrequencyTimes
-	campaignLeaderFrequencyTimes = times
-	return before
-}
diff --git a/tests/integrations/client/client_test.go b/tests/integrations/client/client_test.go
@@ -45,7 +45,6 @@ import (
 	"github.com/tikv/pd/pkg/core"
 	"github.com/tikv/pd/pkg/errs"
 	"github.com/tikv/pd/pkg/mcs/utils"
-	"github.com/tikv/pd/pkg/member"
 	"github.com/tikv/pd/pkg/mock/mockid"
 	"github.com/tikv/pd/pkg/storage/endpoint"
 	"github.com/tikv/pd/pkg/tso"
@@ -166,11 +165,11 @@ func TestClientLeaderChange(t *testing.T) {
 }
 
 func TestLeaderTransferAndMoveCluster(t *testing.T) {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := require.New(t)
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := require.New(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	cluster, err := tests.NewTestCluster(ctx, 3)

diff --git a/tests/integrations/mcs/scheduling/server_test.go b/tests/integrations/mcs/scheduling/server_test.go
@@ -30,7 +30,6 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/tikv/pd/pkg/core/storelimit"
 	mcs "github.com/tikv/pd/pkg/mcs/utils"
-	"github.com/tikv/pd/pkg/member"
 	"github.com/tikv/pd/pkg/schedule/operator"
 	"github.com/tikv/pd/pkg/schedule/schedulers"
 	"github.com/tikv/pd/pkg/utils/testutil"
@@ -652,11 +651,13 @@ func (suite *multipleServerTestSuite) TearDownSuite() {
 }
 
 func (suite *multipleServerTestSuite) TestReElectLeader() {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := suite.Require()
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		// reset the failpoint
+		re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(3)"))
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := suite.Require()
 	tc, err := tests.NewTestSchedulingCluster(suite.ctx, 1, suite.backendEndpoints)
 	re.NoError(err)
 	defer tc.Destroy()

diff --git a/tests/server/cluster/cluster_test.go b/tests/server/cluster/cluster_test.go
@@ -34,7 +34,6 @@ import (
 	"github.com/tikv/pd/pkg/core/storelimit"
 	"github.com/tikv/pd/pkg/dashboard"
 	"github.com/tikv/pd/pkg/id"
-	"github.com/tikv/pd/pkg/member"
 	"github.com/tikv/pd/pkg/mock/mockid"
 	sc "github.com/tikv/pd/pkg/schedule/config"
 	"github.com/tikv/pd/pkg/schedule/operator"
@@ -184,11 +183,11 @@ func TestDamagedRegion(t *testing.T) {
 }
 
 func TestRegionStatistics(t *testing.T) {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := require.New(t)
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := require.New(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tc, err := tests.NewTestCluster(ctx, 3)
@@ -1647,11 +1646,11 @@ func TestMinResolvedTS(t *testing.T) {
 
 // See https://github.com/tikv/pd/issues/4941
 func TestTransferLeaderBack(t *testing.T) {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := require.New(t)
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := require.New(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	tc, err := tests.NewTestCluster(ctx, 2)

diff --git a/tests/server/id/id_test.go b/tests/server/id/id_test.go
@@ -19,9 +19,9 @@ import (
 	"sync"
 	"testing"
 
+	"github.com/pingcap/failpoint"
 	"github.com/pingcap/kvproto/pkg/pdpb"
 	"github.com/stretchr/testify/require"
-	"github.com/tikv/pd/pkg/member"
 	"github.com/tikv/pd/pkg/utils/syncutil"
 	"github.com/tikv/pd/pkg/utils/testutil"
 	"github.com/tikv/pd/tests"
@@ -107,11 +107,11 @@ func TestCommand(t *testing.T) {
 }
 
 func TestMonotonicID(t *testing.T) {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := require.New(t)
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := require.New(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	cluster, err := tests.NewTestCluster(ctx, 2)

diff --git a/tests/server/region_syncer/region_syncer_test.go b/tests/server/region_syncer/region_syncer_test.go
@@ -23,7 +23,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tikv/pd/pkg/core"
-	"github.com/tikv/pd/pkg/member"
 	"github.com/tikv/pd/pkg/utils/testutil"
 	"github.com/tikv/pd/server/config"
 	"github.com/tikv/pd/tests"
@@ -255,11 +254,13 @@ func TestPrepareChecker(t *testing.T) {
 
 // ref: https://github.com/tikv/pd/issues/6988
 func TestPrepareCheckerWithTransferLeader(t *testing.T) {
-	beforeTimes := member.ChangeFrequencyTimes(10)
+	re := require.New(t)
+	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(10)"))
 	defer func() {
-		member.ChangeFrequencyTimes(beforeTimes)
+		// reset the failpoint
+		re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/member/changeFrequencyTimes", "return(3)"))
+		re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/member/changeFrequencyTimes"))
 	}()
-	re := require.New(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/schedule/changeCoordinatorTicker", `return(true)`))