min-resolved-ts: check dc label #944
Conversation
Signed-off-by: husharp <[email protected]>
HuSharp
force-pushed
the
check_dc_label
branch
from
73ece08
to
6a19c69
Compare
HuSharp
force-pushed
the
check_dc_label
branch
2 times, most recently
from
6352532
to
8da079b
Compare
Signed-off-by: husharp <[email protected]>
HuSharp
force-pushed
the
check_dc_label
branch
from
8da079b
to
2332614
Compare
seems like is a non-related flaky fai... we can check another pr meeting it as well. https://github.com/tikv/client-go/actions/runs/5873468298/job/15926773544?pr=947 |
tikv/kv.go
Outdated
| @@ -601,18 +592,40 @@ func (s *KVStore) updateSafeTS(ctx context.Context) { | |||
| }(ctx, wg, storeID, storeAddr) | |||
| } | |||
|
|
|||
| txnScopeMap := make(map[string][]uint64) | |||
| if clusterTS != 0 && err == nil { | |||
There was a problem hiding this comment.
Its better improve the readable like:
clusterMinSafeTS, txnScopeMap, err := s.buildTxnScopeMap(ctx, stores)
// fast path get cluster scope MinSafeTs
if clusterMinSafeTS!= 0 && err == nil {
s.minSafeTS.Store(oracle.GlobalTxnScope, clusterMinSafeTS)
return
}
// Get SafeTS from TiKV by Store
for _, store := range stores {
...
}
Signed-off-by: husharp <[email protected]>
Signed-off-by: husharp <[email protected]>
|
I check out the flacky test, can you help change L255 of the test into I will update other branches. |
Signed-off-by: husharp <[email protected]>
fixed |
tikv/kv.go
Outdated
| // - if stores label are global, return get cluster min resolved ts from pd. | ||
| // - if contains dc label store, return try to get it from TiKV. | ||
| func (s *KVStore) buildTxnScopeMap(ctx context.Context, stores []*Store) (safeTS uint64, txnScopeMap map[string][]uint64, err error) { | ||
| isGlobal := true |
There was a problem hiding this comment.
We do not need judge from the tikv labels, I think should judge from the config like the usage in tidb:
https://github.com/pingcap/tidb/blob/88225787f3c3da18323415c72208bce20876009a/expression/builtin_time.go#L6617-L6632
HuSharp
force-pushed
the
check_dc_label
branch
from
abff1dd
to
835d418
Compare
Signed-off-by: husharp <[email protected]>
HuSharp
force-pushed
the
check_dc_label
branch
from
835d418
to
e23033f
Compare
| if s.pdHttpClient != nil && isGlobal { | ||
| clusterMinSafeTS, err := s.pdHttpClient.GetClusterMinResolvedTS(ctx) | ||
| if err != nil { | ||
| logutil.BgLogger().Debug("get cluster-level min resolved timestamp from PD failed", zap.Error(err)) |
There was a problem hiding this comment.
I think we can warn it, it is called every 2 secs.
There was a problem hiding this comment.
For clusters upgraded from v6.0.0~v6.2.0 will meet this error, does it make sense?
https://docs.pingcap.com/tidb/stable/pd-configuration-file#min-resolved-ts-persistence-interval-new-in-v600
HuSharp
force-pushed
the
check_dc_label
branch
from
e78bf28
to
e23033f
Compare
Background
api interface
Nowadays we have 2 api interfaces for obtaining min resolved ts
For client-go's updateSafeTS, we call
approach IIfor each store, which is not necessary to call API so many timesWe can use the
approach Ito get cluster's min resolved ts when@@txn_scopeisglobalavoid using kv request reason:
if TiDB failed to connect a TiKV store to fetch the latest resolved_ts, it will remain the out-of-dated value until the TiKV comes back, though PD will mark the store as unavailable and keep updating the minimal resolved_ts with other TiKV stores. We may need to keep these two behaviors the same or unify them.
Summary pr
judge
@@txn_scopevalue which from the config:global, we can get cluster-level resolved ts to avoid using kv grpc.global, we need to use kv grpc.