-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[vtadmin] Add vtctld proxy to vtadmin API, add GetKeyspaces endpoint #7266
Conversation
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
This will allow both vtsql and vtctldclient to access it Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…, and vtadmin Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
"vitess.io/vitess/go/vt/vitessdriver" | ||
"vitess.io/vitess/go/vt/vtadmin/cluster" | ||
"vitess.io/vitess/go/vt/vtadmin/cluster/discovery/fakediscovery" | ||
"vitess.io/vitess/go/vt/vtadmin/grpcserver" | ||
"vitess.io/vitess/go/vt/vtadmin/http" | ||
vtadminvtctldclient "vitess.io/vitess/go/vt/vtadmin/vtctldclient" |
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.
This is, ahem, admittedly extremely gross. Please help lol
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 could call it vtctldclient
given that we're already in a "vtadmin" context. 🧐 Up to you though, I don't think this is any worse than all the other vtctld-related consonant salads lol.
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.
It conflicts with the import on line 40
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.
w e l p :')
c4d91c5
to
d6faf28
Compare
d6faf28
to
2a032aa
Compare
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
This PR is still in progress right? 🐈 No rush obviously, just wanna make sure you're not waiting on me for reviews! :') (It looks great so far, but what else is new!) |
It's "in-progress" only in the sense that I may need to make some minor tweaks to iron out any bugs after deploying (for ex yesterday I found "oh right, I never made this reachable over HTTP"). But! I feel good enough about the rest of the implementation (basically everything below the layers in api.go. |
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.
This looks great! (And... familiar! hehehe). I encourage you to get another golang +1, though, since this PR is so big. :)
Tomorrow I'll run this locally using static file discovery (so we can say we can say we did, and because I'll have to for vtadmin-web anyway). I'll also update the vtadmin-web README until we get a better local dev env set up. :') After that, I'll officially approve this bad boy. ✨
Thank you so much for doing this! I'm so excited for our upcoming vrep endpoints!!!
// further. Which vtctld in a set of found vtctlds is returned is not | ||
// specified by the interface, and can be implementation-specific. | ||
DiscoverVtctld(ctx context.Context, tags []string) (*vtadminpb.Vtctld, error) | ||
// DiscoverVtctldAddr returns the address of a of vtctld found in the |
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.
Typo: "of a of"
DiscoverVtctld(ctx context.Context, tags []string) (*vtadminpb.Vtctld, error) | ||
// DiscoverVtctldAddr returns the address of a of vtctld found in the | ||
// discovery service. Tags can optionally be used to filter the set of | ||
// potential vtctld further. Which gate in a set of found vtctld is used to |
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.
Typo: "potential vtctlds" + "found vtctlds"
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.
And also "which gate" whoops!
@@ -52,11 +52,16 @@ type StaticFileDiscovery struct { | |||
byName map[string]*vtadminpb.VTGate | |||
byTag map[string][]*vtadminpb.VTGate | |||
} | |||
vtctlds struct { |
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 for making the changes to this file (even though I guess it was uh, mandatory) 🙏
) | ||
|
||
// GetKeyspaces implements the http wrapper for /keyspaces[?cluster=[&cluster=]]. | ||
func GetKeyspaces(ctx context.Context, r Request, api *API) *JSONResponse { |
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.
😻
Signed-off-by: Andrew Mason <amason@slack-corp.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.
lgtm
|
||
count := len(vtctlds) | ||
if count == 0 { | ||
return nil, ErrNoVTGates |
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.
Sorry for missing this before -- I think this should be ErrNoVtctlds
.
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.
Fixed! Nice catch!
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.
Apart from this tiny comment, this (still) looks great. ✨
I was able to get it almost but not entirely working against local Docker/Vitess + vtadmin-web... I think I'm missing a couple of flags. 🤔 Given that you deployed this internally + it is working great, I don't think we should block this merge on me. We can figure it out when you're back. 😊
Happy to help with flags today (or later)! |
Signed-off-by: Andrew Mason <amason@slack-corp.com>
[vtadmin] Add vtctld proxy to vtadmin API, add GetKeyspaces endpoint
(I still need to deploy this fully to test, hence draft. But all feedback welcome!!)Confirmed it works, yay!Description
This does the following:
vtctldclient.Proxy
to vtadmin, semantically similar tovtsql.DB
but for vtctldclient connections.Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: