NotLeader error may cause unnecessary backoff by causing pseudo region error

[MyonKeminta]

When a NotLeader error carrying a new leader info is received from TiKV, we update the leader info of the region by updating the struct Region 's store field which is in type RegionStore). As the Region is a type of shared data managed by RegionCache, the store is updated by replacing atomic pointers, and thus RegionStore is used in copy-on-write pattern.

client-go/internal/locate/region_cache.go

Lines 2451 to 2454 in abda04c

	newRegionStore := oldRegionStore.clone()
	newRegionStore.workTiKVIdx = leaderIdx
	newRegionStore.storeEpochs[leaderIdx] = atomic.LoadUint32(&newRegionStore.stores[leaderIdx].epoch)
	if !r.compareAndSwapStore(oldRegionStore, newRegionStore) {

Then, #264 introduces this which is executed every time attempting to send a request:

client-go/internal/locate/region_request.go

Lines 1023 to 1026 in abda04c

	if &oldRegionStore.stores != &newRegionStore.stores {
	s.state = &invalidStore{}
	return
	}

This forces the replicaSelector to be rebuilt when retrying on NotLeader error, by return nil rpcCtx:

client-go/internal/locate/region_request.go

Lines 894 to 897 in abda04c

	func (state *invalidStore) next(_ *retry.Backoffer, _ *replicaSelector) (*RPCContext, error) {
	metrics.TiKVReplicaSelectorFailureCounter.WithLabelValues("invalidStore").Inc()
	return nil, nil
	}

and then triggering pseudo region error:

client-go/internal/locate/region_request.go

Lines 1424 to 1435 in abda04c

	if rpcCtx == nil {
	// TODO(youjiali1995): remove it when using the replica selector for all requests.
	// If the region is not found in cache, it must be out
	// of date and already be cleaned up. We can skip the
	// RPC by returning RegionError directly.
	// TODO: Change the returned error to something like "region missing in cache",
	// and handle this error like EpochNotMatch, which means to re-split the request and retry.
	s.logSendReqError(bo, "throwing pseudo region error due to no replica available", regionID, retryTimes, req, totalErrors)
	resp, err = tikvrpc.GenRegionErrorResp(req, &errorpb.Error{EpochNotMatch: &errorpb.EpochNotMatch{}})
	return resp, nil, retryTimes, err
	}

This will cause backoff to the caller of SendReqCtx, which is unnecessary in a NotLeader case where the new leader is immediately known.

The usage of invalidStore introduced by #264 mentioned above looks expected to compare whether the store field of the old and new regionStore instances points to the same block of memory, but mistakenly compared the address of the store field itself.