Merge pull request #585 from cybozu-go/fix-findtoprunner

Remove copy code of FindTopRunner
cybozu-go · Oct 20, 2023 · a977d93 · a977d93
2 parents ee3134d + a475550
commit a977d93
Show file tree

Hide file tree

Showing 6 changed files with 10 additions and 75 deletions.
diff --git a/clustering/mock_test.go b/clustering/mock_test.go
@@ -217,65 +217,6 @@ func (o *mockOperator) IsSubsetGTID(ctx context.Context, set1, set2 string) (boo
 	return len(map1) == 0, nil
 }
 
-// FindTopRunner returns the index of the slice whose `GlobalVariables.ExecutedGtidSet`
-// is most advanced.  This may return ErrErrantTransactions for errant transactions
-// or ErrNoTopRunner if there is no such instance.
-func (o *mockOperator) FindTopRunner(ctx context.Context, status []*dbop.MySQLInstanceStatus) (int, error) {
-	if o.failing {
-		return -1, errors.New("mysqld is down")
-	}
-
-	latest := -1
-	var latestGTID string
-
-	for i := 0; i < len(status); i++ {
-		if status[i] == nil {
-			continue
-		}
-		repl := status[i].ReplicaStatus
-		if repl == nil {
-			continue
-		}
-
-		gtid := repl.RetrievedGtidSet
-		if len(gtid) == 0 {
-			continue
-		}
-
-		if len(latestGTID) == 0 {
-			latest = i
-			latestGTID = gtid
-			continue
-		}
-
-		isSubset, err := o.IsSubsetGTID(ctx, gtid, latestGTID)
-		if err != nil {
-			return -1, err
-		}
-		if isSubset {
-			continue
-		}
-
-		isSubset, err = o.IsSubsetGTID(ctx, latestGTID, gtid)
-		if err != nil {
-			return -1, err
-		}
-		if isSubset {
-			latest = i
-			latestGTID = gtid
-			continue
-		}
-
-		return -1, fmt.Errorf("%w: set1=%s, set2=%s", dbop.ErrErrantTransactions, gtid, latestGTID)
-	}
-
-	if latest == -1 {
-		return -1, dbop.ErrNoTopRunner
-	}
-
-	return latest, nil
-}
-
 // ConfigureReplica configures client-side replication.
 // If `symisync` is true, it enables client-side semi-synchronous replication.
 // In either case, it disables server-side semi-synchronous replication.

diff --git a/clustering/operations.go b/clustering/operations.go
@@ -218,7 +218,7 @@ func (p *managerProcess) failover(ctx context.Context, ss *StatusSet) error {
 		candidates[i] = newStatus
 	}
 
-	candidate, err := op.FindTopRunner(ctx, candidates)
+	candidate, err := dbop.FindTopRunner(ctx, op, candidates)
 	if err != nil {
 		return fmt.Errorf("failed to choose the next primary: %w", err)
 	}

diff --git a/pkg/dbop/gtid.go b/pkg/dbop/gtid.go
@@ -5,7 +5,10 @@ import (
 	"fmt"
 )
 
-func (o *operator) FindTopRunner(ctx context.Context, status []*MySQLInstanceStatus) (int, error) {
+// FindTopRunner returns the index of the slice whose `GlobalVariables.ExecutedGtidSet`
+// is most advanced.  This may return ErrErrantTransactions for errant transactions
+// or ErrNoTopRunner if there is no such instance.
+func FindTopRunner(ctx context.Context, o Operator, status []*MySQLInstanceStatus) (int, error) {
 	latest := -1
 	var latestGTIDs string
 

diff --git a/pkg/dbop/gtid_test.go b/pkg/dbop/gtid_test.go
@@ -24,7 +24,7 @@ var _ = Describe("FindTopRunner", func() {
 		defer op.Close()
 
 		statuses := make([]*MySQLInstanceStatus, 3)
-		_, err = op.FindTopRunner(context.Background(), statuses)
+		_, err = FindTopRunner(context.Background(), op, statuses)
 		Expect(err).To(MatchError(ErrNoTopRunner))
 
 		set0 := `8e349184-bc14-11e3-8d4c-0800272864ba:1-29`
@@ -33,21 +33,21 @@ var _ = Describe("FindTopRunner", func() {
 		statuses[0] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set0}}
 		statuses[1] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set1}}
 		statuses[2] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set2}}
-		top, err := op.FindTopRunner(context.Background(), statuses)
+		top, err := FindTopRunner(context.Background(), op, statuses)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(top).To(Equal(2))
 
 		statuses[0] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set2}}
 		statuses[1] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set0}}
 		statuses[2] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set1}}
-		top, err = op.FindTopRunner(context.Background(), statuses)
+		top, err = FindTopRunner(context.Background(), op, statuses)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(top).To(Equal(0))
 
 		statuses[0] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set1}}
 		statuses[1] = nil
 		statuses[2] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set2}}
-		top, err = op.FindTopRunner(context.Background(), statuses)
+		top, err = FindTopRunner(context.Background(), op, statuses)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(top).To(Equal(2))
 
@@ -59,7 +59,7 @@ var _ = Describe("FindTopRunner", func() {
 		statuses[0] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set0}}
 		statuses[1] = &MySQLInstanceStatus{ReplicaStatus: &ReplicaStatus{RetrievedGtidSet: set1}}
 		statuses[2] = nil
-		_, err = op.FindTopRunner(context.Background(), statuses)
+		_, err = FindTopRunner(context.Background(), op, statuses)
 		Expect(err).To(MatchError(ErrErrantTransactions))
 	})
 })
diff --git a/pkg/dbop/nop.go b/pkg/dbop/nop.go
@@ -35,10 +35,6 @@ func (o NopOperator) IsSubsetGTID(ctx context.Context, set1, set2 string) (bool,
 	return false, ErrNop
 }
 
-func (o NopOperator) FindTopRunner(context.Context, []*MySQLInstanceStatus) (int, error) {
-	return 0, ErrNop
-}
-
 func (o NopOperator) ConfigureReplica(ctx context.Context, source AccessInfo, semisync bool) error {
 	return ErrNop
 }

diff --git a/pkg/dbop/operator.go b/pkg/dbop/operator.go
@@ -37,11 +37,6 @@ type Operator interface {
 	// IsSubsetGTID returns true if set1 is a subset of set2.
 	IsSubsetGTID(ctx context.Context, set1, set2 string) (bool, error)
 
-	// FindTopRunner returns the index of the slice whose `GlobalVariables.ExecutedGtidSet`
-	// is most advanced.  This may return ErrErrantTransactions for errant transactions
-	// or ErrNoTopRunner if there is no such instance.
-	FindTopRunner(context.Context, []*MySQLInstanceStatus) (int, error)
-
 	// ConfigureReplica configures client-side replication.
 	// If `symisync` is true, it enables client-side semi-synchronous replication.
 	// In either case, it disables server-side semi-synchronous replication.