feat: make fixSemiSync use the durability policy information

Signed-off-by: Manan Gupta <[email protected]>
vitessio · Jan 22, 2022 · 1380e76 · 1380e76
1 parent ddcaa78
commit 1380e76
Show file tree

Hide file tree

Showing 2 changed files with 4 additions and 140 deletions.
diff --git a/go/vt/vttablet/tabletmanager/rpc_replication.go b/go/vt/vttablet/tabletmanager/rpc_replication.go
@@ -885,29 +885,12 @@ func isPrimaryEligible(tabletType topodatapb.TabletType) bool {
 }
 
 func (tm *TabletManager) fixSemiSync(tabletType topodatapb.TabletType, semiSync SemiSyncAction) error {
-	if !*enableSemiSync {
-		// Semi-sync handling is not enabled.
-		if semiSync == SemiSyncActionTrue {
-			log.Error("invalid configuration - semi-sync should be setup according to durability policies, but enable_semi_sync is not set")
-		}
-		return nil
-	}
-
-	// Only enable if we're eligible for becoming primary (REPLICA type).
-	// Ineligible tablets (RDONLY) shouldn't ACK because we'll never promote them.
-	if !isPrimaryEligible(tabletType) {
-		if semiSync == SemiSyncActionTrue {
-			log.Error("invalid configuration - semi-sync should be setup according to durability policies, but the tablet is not primaryEligible")
-		}
+	if semiSync == SemiSyncActionTrue {
+		return tm.MysqlDaemon.SetSemiSyncEnabled(tabletType == topodatapb.TabletType_PRIMARY, true)
+	} else if semiSync == SemiSyncActionFalse {
 		return tm.MysqlDaemon.SetSemiSyncEnabled(false, false)
 	}
-
-	if semiSync == SemiSyncActionFalse {
-		log.Error("invalid configuration - enabling semi sync even though not specified by durability policies. Possibly in the process of upgrading.")
-	}
-	// Always enable replica-side since it doesn't hurt to keep it on for a primary.
-	// The primary-side needs to be off for a replica, or else it will get stuck.
-	return tm.MysqlDaemon.SetSemiSyncEnabled(tabletType == topodatapb.TabletType_PRIMARY, true)
+	return nil
 }
 
 func (tm *TabletManager) isPrimarySideSemiSyncEnabled() bool {

diff --git a/go/vt/vttablet/tabletmanager/rpc_replication_test.go b/go/vt/vttablet/tabletmanager/rpc_replication_test.go
@@ -17,16 +17,11 @@ limitations under the License.
 package tabletmanager
 
 import (
-	"bytes"
 	"context"
-	"io"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
-	"vitess.io/vitess/go/vt/mysqlctl/fakemysqldaemon"
-	"vitess.io/vitess/go/vt/proto/topodata"
 	"vitess.io/vitess/go/vt/topo/memorytopo"
 )
 
@@ -43,117 +38,3 @@ func TestPromoteReplicaHealthTicksStopped(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, tm.replManager.ticks.Running())
 }
-
-func captureStderr(f func()) (string, error) {
-	old := os.Stderr // keep backup of the real stderr
-	r, w, err := os.Pipe()
-	if err != nil {
-		return "", err
-	}
-	os.Stderr = w
-
-	outC := make(chan string)
-	// copy the output in a separate goroutine so printing can't block indefinitely
-	go func() {
-		var buf bytes.Buffer
-		io.Copy(&buf, r)
-		outC <- buf.String()
-	}()
-
-	// calling function which stderr we are going to capture:
-	f()
-
-	// back to normal state
-	w.Close()
-	os.Stderr = old // restoring the real stderr
-	return <-outC, nil
-}
-
-func TestTabletManager_fixSemiSync(t *testing.T) {
-	tests := []struct {
-		name                 string
-		tabletType           topodata.TabletType
-		semiSync             SemiSyncAction
-		logOutput            string
-		shouldEnableSemiSync bool
-	}{
-		{
-			name:                 "enableSemiSync=true(primary eligible),durabilitySemiSync=true",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionTrue,
-			logOutput:            "",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=true(primary eligible),durabilitySemiSync=false",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionFalse,
-			logOutput:            "invalid configuration - enabling semi sync even though not specified by durability policies.",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=true(primary eligible),durabilitySemiSync=none",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionNone,
-			logOutput:            "",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=true(primary not-eligible),durabilitySemiSync=true",
-			tabletType:           topodata.TabletType_DRAINED,
-			semiSync:             SemiSyncActionTrue,
-			logOutput:            "invalid configuration - semi-sync should be setup according to durability policies, but the tablet is not primaryEligible",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=true(primary not-eligible),durabilitySemiSync=false",
-			tabletType:           topodata.TabletType_DRAINED,
-			semiSync:             SemiSyncActionFalse,
-			logOutput:            "",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=true(primary not-eligible),durabilitySemiSync=none",
-			tabletType:           topodata.TabletType_DRAINED,
-			semiSync:             SemiSyncActionNone,
-			logOutput:            "",
-			shouldEnableSemiSync: true,
-		}, {
-			name:                 "enableSemiSync=false,durabilitySemiSync=true",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionTrue,
-			logOutput:            "invalid configuration - semi-sync should be setup according to durability policies, but enable_semi_sync is not set",
-			shouldEnableSemiSync: false,
-		}, {
-			name:                 "enableSemiSync=false,durabilitySemiSync=false",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionFalse,
-			logOutput:            "",
-			shouldEnableSemiSync: false,
-		}, {
-			name:                 "enableSemiSync=false,durabilitySemiSync=none",
-			tabletType:           topodata.TabletType_REPLICA,
-			semiSync:             SemiSyncActionNone,
-			logOutput:            "",
-			shouldEnableSemiSync: false,
-		},
-	}
-	oldEnableSemiSync := *enableSemiSync
-	defer func() {
-		*enableSemiSync = oldEnableSemiSync
-	}()
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			*enableSemiSync = tt.shouldEnableSemiSync
-			fakeMysql := fakemysqldaemon.NewFakeMysqlDaemon(nil)
-			tm := &TabletManager{
-				MysqlDaemon: fakeMysql,
-			}
-			logOutput, err := captureStderr(func() {
-				err := tm.fixSemiSync(tt.tabletType, tt.semiSync)
-				require.NoError(t, err)
-			})
-			require.NoError(t, err)
-			if tt.logOutput != "" {
-				require.Contains(t, logOutput, tt.logOutput)
-			} else {
-				require.Equal(t, "", logOutput)
-			}
-		})
-	}
-}