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

Introduce the RURuntimeStats #732

Merged
merged 12 commits into from
Mar 16, 2023
Merged

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Mar 8, 2023

Ref pingcap/tidb#41972. Should merge after pingcap/kvproto#1073 and tikv/pd#6125.

Before EXPLAIN ANALYZE is executed, it will set RURuntimeStats with its start_ts, then the OnRequest and OnResponse could get the corresponding RURuntimeStats and update its RU info.

Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: JmPotato <ghzpotato@gmail.com>
tikv/kv.go Show resolved Hide resolved
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@@ -391,6 +391,7 @@ func (s *KVSnapshot) batchGetSingleRegion(bo *retry.Backoffer, batch batchKeys,
Keys: pending,
Version: s.version,
}, s.mu.replicaRead, &s.replicaReadSeed, kvrpcpb.Context{
StartTs: s.version,
Copy link
Member

@Connor1996 Connor1996 Mar 14, 2023

Choose a reason for hiding this comment

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

All the places that you set StartTs are just a copy of start version in each type of command. How about getting the start ts with a match of the request type instead of adding a new field in context

}

func (r interceptedClient) SendRequest(ctx context.Context, addr string, req *tikvrpc.Request, timeout time.Duration) (*tikvrpc.Response, error) {
// Build the resource control interceptor.
resourceGroupName := req.GetResourceGroupName()
var finalInterceptor interceptor.RPCInterceptor = buildResourceControlInterceptor(ctx, req, resourceGroupName)
var finalInterceptor interceptor.RPCInterceptor = buildResourceControlInterceptor(ctx, req, r.getRURuntimeStats(req.GetStartTs()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder point get use same ts, if run point get workload and run a explain analazy may get wrong result?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the TiDB code and found that the point get inside an EXPLAIN ANALYZE would not use max uint64 due to the following code:

https://github.com/pingcap/tidb/blob/cc56b21242a2ad82a2f079d7f518606f8705fec4/sessiontxn/isolation/optimistic.go#L132

It will not be regarded as a pure point get since it's an EXPLAIN SQL after all. So its start_ts will remain unique.

tikv/kv.go Outdated Show resolved Hide resolved
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato JmPotato requested review from tiancaiamao and glorv and removed request for tiancaiamao March 15, 2023 02:49
@JmPotato
Copy link
Member Author

/cc @tiancaiamao @glorv @nolouch @Connor1996 PTAL again, thx!

Signed-off-by: JmPotato <ghzpotato@gmail.com>
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm. ptal @Connor1996

@JmPotato JmPotato requested review from tiancaiamao and Connor1996 and removed request for glorv and tiancaiamao March 15, 2023 07:36
Copy link
Contributor

@glorv glorv left a comment

Choose a reason for hiding this comment

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

LGTM

@disksing disksing merged commit 9d95090 into tikv:master Mar 16, 2023
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.

6 participants