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

Add start_ts field into kvrpcpb.Context #1073

Closed
wants to merge 1 commit into from

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Mar 9, 2023

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@@ -845,6 +845,9 @@ message Context {
// error before processing if the estimated waiting duration exceeds the threshold.
uint32 busy_threshold_ms = 27;

// The `start_ts` of the transaction that the request belongs to.
uint64 start_ts = 28;
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.

I notice a lot of requests already have a start_version field. Why bother to add one more in context?

Copy link
Member Author

Choose a reason for hiding this comment

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

After digging into the code, I think adding a start_ts field is the simplest way to get it passed into the place where the resource control works at.

Please take a look at https://github.com/tikv/client-go/pull/732/files#diff-34812587e42268a5d84933be286601fc58fdff17ab336b09ba75455ffe5ed447R47. tikvrpc.Request contains kvrpcpb.Context.

Copy link
Member

Choose a reason for hiding this comment

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

But you can get the start ts from tikvrpc.Request by a type matching

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. Let me try it out.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, The PointGet may use max_ts? Will this affect the 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 double-checked the code of TiDB and think it's not a problem. Here is the reason: tikv/client-go#732 (comment).

@JmPotato
Copy link
Member Author

/hold

@JmPotato JmPotato marked this pull request as draft March 15, 2023 03:02
@JmPotato JmPotato closed this Mar 17, 2023
@JmPotato JmPotato deleted the ru_runtime_stats branch March 17, 2023 05:39
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.

4 participants