-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 GetServerStatus
RPC to use in PRS
#16022
Conversation
Signed-off-by: Manan Gupta <manan@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16022 +/- ##
==========================================
- Coverage 68.24% 68.21% -0.04%
==========================================
Files 1541 1541
Lines 197117 197305 +188
==========================================
+ Hits 134530 134595 +65
- Misses 62587 62710 +123 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <manan@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @GuptaManan100 ! I only had some nits and you can address those are you feel is best.
return nil, err | ||
} | ||
} | ||
qr, err := mysqld.FetchSuperQuery(ctx, query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's worth creating a mysqld.FetchQuery
helper (doesn't have to be done here) as there should be no need to use the DBA pool for this and various other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that change as a follow up. Currently rushing to get this into RC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from @mattlord seem worth addressing. Rest LGTM.
Leaving this to @GuptaManan100 to merge once remaining feedback has been addressed. |
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
DCO seems to be stuck everywhere. I've manually verified that all commits have the necessary signoff. Bypassing that check to merge. |
Description
This PR starts the work for #15946.
In order to use the replica which has the innodb buffer pool loaded the most, we first need the ability to read that said value. This PR adds a new RPC to the tablet manager client interface for this.
In the next release, when this RPC is available on all the vttablets, we can use this RPC instead of
PrimaryStatus
to check for the availability of all the replicas in PRS. Currently we are throwing away the results ofPrimaryStatus
. Instead we can callGetServerStatus
and even use the results for better choosing the primary candidate.Related Issue(s)
Checklist
Deployment Notes