only set MinSafeTS to 0 when all stores' are 0 (#1284)

* only set MinSafeTS to 0 when all stores' are 0 Signed-off-by: zyguan <[email protected]> * try to make ut stable Signed-off-by: zyguan <[email protected]> --------- Signed-off-by: zyguan <[email protected]>
tikv · Apr 18, 2024 · 713ceee · 713ceee
1 parent f959104
commit 713ceee
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 15 deletions.
diff --git a/internal/logutil/log.go b/internal/logutil/log.go
@@ -36,6 +36,7 @@ package logutil
 
 import (
 	"context"
+	"testing"
 
 	"github.com/pingcap/log"
 	"go.uber.org/zap"
@@ -60,3 +61,11 @@ type ctxLogKeyType struct{}
 // CtxLogKey is the key to retrieve logger from context.
 // It can be assigned to another value.
 var CtxLogKey interface{} = ctxLogKeyType{}
+
+// AssertWarn panics when in testing mode, and logs a warning msg otherwise.
+func AssertWarn(logger *zap.Logger, msg string, fields ...zap.Field) {
+	if testing.Testing() {
+		logger.Panic(msg, fields...)
+	}
+	logger.Warn(msg, fields...)
+}
diff --git a/tikv/kv.go b/tikv/kv.go
@@ -44,7 +44,6 @@ import (
 	"strings"
 	"sync"
 	"sync/atomic"
-	"testing"
 	"time"
 
 	"github.com/opentracing/opentracing-go"
@@ -576,11 +575,7 @@ func (s *KVStore) GetMinSafeTS(txnScope string) uint64 {
 func (s *KVStore) setMinSafeTS(txnScope string, safeTS uint64) {
 	// ensure safeTS is not set to max uint64
 	if safeTS == math.MaxUint64 {
-		if testing.Testing() {
-			panic(fmt.Errorf("unreachable"))
-		} else {
-			logutil.BgLogger().Warn("skip setting min-safe-ts to max uint64", zap.String("txnScope", txnScope), zap.Stack("stack"))
-		}
+		logutil.AssertWarn(logutil.BgLogger(), "skip setting min-safe-ts to max uint64", zap.String("txnScope", txnScope), zap.Stack("stack"))
 		return
 	}
 	s.minSafeTS.Store(txnScope, safeTS)
@@ -623,11 +618,7 @@ func (s *KVStore) getSafeTS(storeID uint64) (bool, uint64) {
 func (s *KVStore) setSafeTS(storeID, safeTS uint64) {
 	// ensure safeTS is not set to max uint64
 	if safeTS == math.MaxUint64 {
-		if testing.Testing() {
-			panic(fmt.Errorf("unreachable"))
-		} else {
-			logutil.BgLogger().Warn("skip setting safe-ts to max uint64", zap.Uint64("storeID", storeID), zap.Stack("stack"))
-		}
+		logutil.AssertWarn(logutil.BgLogger(), "skip setting safe-ts to max uint64", zap.Uint64("storeID", storeID), zap.Stack("stack"))
 		return
 	}
 	s.safeTSMap.Store(storeID, safeTS)
@@ -646,13 +637,17 @@ func (s *KVStore) updateMinSafeTS(txnScope string, storeIDs []uint64) {
 		ok, safeTS := s.getSafeTS(store)
 		if ok {
 			// safeTS is guaranteed to be less than math.MaxUint64 (by setSafeTS and its callers)
-			if safeTS < minSafeTS {
+			if safeTS != 0 && safeTS < minSafeTS {
 				minSafeTS = safeTS
 			}
 		} else {
 			minSafeTS = 0
 		}
 	}
+	// if minSafeTS is still math.MaxUint64, that means all store safe ts are 0, then we set minSafeTS to 0.
+	if minSafeTS == math.MaxUint64 {
+		minSafeTS = 0
+	}
 	s.setMinSafeTS(txnScope, minSafeTS)
 }
 

diff --git a/tikv/kv_test.go b/tikv/kv_test.go
@@ -164,7 +164,7 @@ func (s *testKVSuite) TestMinSafeTsFromStores() {
 	s.Require().Equal(mockClient.tikvSafeTs, ts)
 }
 
-func (s *testKVSuite) TestMinSafeTsFromStoresWithZeroSafeTs() {
+func (s *testKVSuite) TestMinSafeTsFromStoresWithAllZeros() {
 	// ref https://github.com/tikv/client-go/issues/1276
 	mockClient := newStoreSafeTsMockClient(s)
 	mockClient.tikvSafeTs = 0
@@ -178,6 +178,19 @@ func (s *testKVSuite) TestMinSafeTsFromStoresWithZeroSafeTs() {
 	s.Require().Equal(uint64(0), s.store.GetMinSafeTS(oracle.GlobalTxnScope))
 }
 
+func (s *testKVSuite) TestMinSafeTsFromStoresWithSomeZeros() {
+	// ref https://github.com/tikv/tikv/issues/13675 & https://github.com/tikv/client-go/pull/615
+	mockClient := newStoreSafeTsMockClient(s)
+	mockClient.tiflashSafeTs = 0
+	s.store.SetTiKVClient(mockClient)
+
+	s.Eventually(func() bool {
+		return atomic.LoadInt32(&mockClient.requestCount) >= 4
+	}, 15*time.Second, time.Second)
+
+	s.Require().Equal(mockClient.tikvSafeTs, s.store.GetMinSafeTS(oracle.GlobalTxnScope))
+}
+
 func (s *testKVSuite) TestMinSafeTsFromPD() {
 	mockClient := newStoreSafeTsMockClient(s)
 	s.store.SetTiKVClient(mockClient)
@@ -229,7 +242,7 @@ func (s *testKVSuite) TestMinSafeTsFromMixed1() {
 	s.Eventually(func() bool {
 		ts := s.store.GetMinSafeTS("z1")
 		s.Require().False(math.MaxUint64 == ts)
-		return ts == uint64(10)
+		return ts == uint64(10) && s.store.GetMinSafeTS(oracle.GlobalTxnScope) == uint64(10)
 	}, 15*time.Second, time.Second)
 	s.Require().GreaterOrEqual(atomic.LoadInt32(&mockClient.requestCount), int32(1))
 	s.Require().Equal(uint64(10), s.store.GetMinSafeTS(oracle.GlobalTxnScope))
@@ -254,7 +267,7 @@ func (s *testKVSuite) TestMinSafeTsFromMixed2() {
 	s.Eventually(func() bool {
 		ts := s.store.GetMinSafeTS("z2")
 		s.Require().False(math.MaxUint64 == ts)
-		return ts == uint64(10)
+		return ts == uint64(10) && s.store.GetMinSafeTS(oracle.GlobalTxnScope) == uint64(10)
 	}, 15*time.Second, time.Second)
 	s.Require().GreaterOrEqual(atomic.LoadInt32(&mockClient.requestCount), int32(1))
 	s.Require().Equal(uint64(10), s.store.GetMinSafeTS(oracle.GlobalTxnScope))