Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: support requests with source label #506

Merged
merged 8 commits into from
Jun 24, 2022

Conversation

you06
Copy link
Contributor

@you06 you06 commented May 18, 2022

ref pingcap/tidb#33963

TiDB sends requests to TiKV in a way that mixes internal and external queries, this PR divides them and sets the tag for requests.

Signed-off-by: you06 <you1474600@gmail.com>
@you06 you06 marked this pull request as ready for review June 21, 2022 08:04
@sticnarf sticnarf self-requested a review June 22, 2022 10:02
Signed-off-by: you06 <you1474600@gmail.com>
// InternalTxnGC is the type of GC txn.
InternalTxnGC = "gc"
// InternalTxnMeta is the type of the miscellaneous meta usage.
InternalTxnMeta = InternalTxnOthers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some sources are client's inner types and applications use client can give their specified types. Also the types are aliased to reduce the metrics.

v = metrics.TiKVSendReqHistogram.WithLabelValues(reqType, storeID, strconv.FormatBool(staleRead))
sendReqHistCache.Store(key, v)
counter = metrics.TiKVSendReqCounter.WithLabelValues(reqType, storeID, strconv.FormatBool(staleRead), counterKey.requestSource)
sendReqCounterCache.Store(histKey, hist)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another metric I've considered is the "request time"(similar to "db time"). The sum duration of requests by all labels.

@@ -327,16 +334,16 @@ func (lr *LockResolver) BatchResolveLocks(bo *retry.Backoffer, locks []*Lock, lo
// commit status.
// 3) Send `ResolveLock` cmd to the lock's region to resolve all locks belong to
// the same transaction.
func (lr *LockResolver) ResolveLocks(bo *retry.Backoffer, callerStartTS uint64, locks []*Lock) (int64, error) {
ttl, _, _, err := lr.resolveLocks(bo, callerStartTS, locks, false, false)
func (lr *LockResolver) ResolveLocks(bo *retry.Backoffer, callerStartTS uint64, locks []*Lock, detail *util.ResolveLockDetail) (int64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be more friendly to client-go users to add a new function ResolveLocksWithDetail instead of modifying the original interface?

if span := opentracing.SpanFromContext(ctx); span != nil && span.Tracer() != nil {
span1 := span.Tracer().StartSpan("tikvSnapshot.get", opentracing.ChildOf(span.Context()))
defer span1.Finish()
opentracing.ContextWithSpan(ctx, span1)
}
if _, err := util.EvalFailpoint("snapshot-get-cache-fail"); err == nil {
if bo.GetCtx().Value("TestSnapshotCache") != nil {
s.mu.RUnlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for?

Copy link
Contributor Author

@you06 you06 Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the unnecessary unlock and lock in line 526 and 540, but we still need to unlock when panic in the failpoint, unless the mutex will be locked and block all the future usage.


// ResolveLockDetail contains the resolve lock detail information.
type ResolveLockDetail struct {
*RequestSource
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit weird to include this in xxxDetail. Can we just pass it by Context?

Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
@@ -88,6 +88,7 @@ func (s *KVStore) splitBatchRegionsReq(bo *Backoffer, keys [][]byte, scatter boo
zap.Uint64("first batch, region ID", batches[0].RegionID.GetID()),
zap.String("first split key", kv.StrKey(batches[0].Keys[0])))
}
bo.SetCtx(util.WithInternalSourceType(bo.GetCtx(), util.InternalTxnMeta))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add this. SplitRegion may be called during prewrite and I think its source should equal to the source of that prewrite.

Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest lgtm

Comment on lines 416 to 420
callerStartTS := opts.CallerStartTS
locks := opts.Locks
detail := opts.Detail
forRead := opts.ForRead
lite := opts.Lite
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe writing them in one line is simpler?

callerStartTS, locks, detail, forRead, lite := opts.CallerStartTS, opts.Locks, opts.Detail, opts.ForRead, opts.Lite

@@ -988,7 +1048,7 @@ func (lr *LockResolver) resolveRegionLocks(bo *retry.Backoffer, l *Lock, region
return nil
}

func (lr *LockResolver) resolveLock(bo *retry.Backoffer, l *Lock, status TxnStatus, lite bool, cleanRegions map[locate.RegionVerID]struct{}) error {
func (lr *LockResolver) resolveLock(bo *retry.Backoffer, l *Lock, status TxnStatus, lite bool, cleanRegions map[locate.RegionVerID]struct{}, async bool) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is async used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for recording resolve details with different types. Removed because the further details are not included in this PR.

Signed-off-by: you06 <you1474600@gmail.com>
Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sticnarf sticnarf merged commit bb026bd into tikv:master Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants