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 PD API to support setting limit to GC safepoint #596

Closed
wants to merge 3 commits into from

Conversation

MyonKeminta
Copy link
Contributor

Support getting and setting limit to GC safepoint, which makes it possible for other services to block GC.

@shafreeck shafreeck requested review from nolouch and overvenus April 7, 2020 08:51
proto/pdpb.proto Outdated
Comment on lines 568 to 569
// UUID v4
bytes uuid = 2;
Copy link
Member

Choose a reason for hiding this comment

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

I thought you want to change id (with the form of "servicename/uuid")?

proto/pdpb.proto Outdated
message GetGCSafePointLimitResponse {
RequestHeader header = 1;
uint64 safe_point = 2;
uint64 min_safe_point_limit = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Why not extend the GetGCSafePointResponse instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. I misunderstood the document here.

@nolouch nolouch requested a review from shafreeck April 8, 2020 04:01
@nolouch
Copy link
Member

nolouch commented Apr 8, 2020

PTAL @shafreeck

overvenus
overvenus previously approved these changes Apr 8, 2020
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

nolouch
nolouch previously approved these changes Apr 8, 2020
@MyonKeminta MyonKeminta dismissed stale reviews from nolouch and overvenus via c12e099 April 9, 2020 09:05
message SetGCSafePointLimitRequest {
RequestHeader header = 1;
// service_name/uuid_v4
bytes id = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding the id and ttl field in the UpdateGCSafepoint method, then it is not necessary to add a new method.

@@ -498,6 +500,7 @@ message GetGCSafePointResponse {
ResponseHeader header = 1;

uint64 safe_point = 2;
uint64 min_safe_point_limit = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the returned safe_point is always the minimum value, so min_safe_point_limit is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TiDB need to check how far it can advance the safepoint before it actually advance it, because it need to do DeleteRange step with the limited safepoint before sending its safepoint to PD.

@MyonKeminta
Copy link
Contributor Author

Use #603 instead. Closed.

@MyonKeminta MyonKeminta deleted the gc-limit branch April 15, 2020 06:36
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