-
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
[vtadmin] Add GetSchema endpoint #7596
Conversation
Signed-off-by: Andrew Mason <amason@slack-corp.com>
…o use it, implement (*vtadmin.API).GetSchema 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>
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.
Thanks so much for doing this + additionally for remembering to generate the TypeScript types ❤️
Looks great as always, obviously -- just one tiny comment that you can probably ignore at this point!
@@ -87,6 +88,7 @@ func NewAPI(clusters []*cluster.Cluster, opts grpcserver.Options, httpOpts vtadm | |||
router.HandleFunc("/clusters", httpAPI.Adapt(vtadminhttp.GetClusters)).Name("API.GetClusters") | |||
router.HandleFunc("/gates", httpAPI.Adapt(vtadminhttp.GetGates)).Name("API.GetGates") | |||
router.HandleFunc("/keyspaces", httpAPI.Adapt(vtadminhttp.GetKeyspaces)).Name("API.GetKeyspaces") | |||
router.HandleFunc("/schema/{cluster_id}/{keyspace}/{table}", httpAPI.Adapt(vtadminhttp.GetSchema)).Name("API.GetSchema") |
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.
What do you think about making this /schema?cluster_id=&keyspace=&table=
? I honestly think the way you have it now is prettier, but:
- It would also work for disambiguation (where cluster/keyspace are optinoal)
- We already do this for the
/tablet
endpoint (which has an optional cluster parameter too)
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.
I realize you started the disambiguation branch anyway so I expect you already have some plan 😎
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.
I think, given that you said this route scheme looks prettier, that we should keep it as-is because:
- we're still able to reuse the logic of
cluster.GetSchema
between the two endpoints, so we're not duplicating too much - having two distinct rpcs and http routes lets us move some of the conditional logic out of the actual function and into the route definition (i.e.
GetSchema
doesn't have to thatlen(clusterIds)
is exactly 1, because it's not possible, but if we combine it with theFindSchema
endpoint then we'll end up introducing a bunch of "if this then do thingAForGetSchema else do thingBForFindSchema" branching)
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.
Yeah makes sense!
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.
The /schema endpoint does seem a bit non-std instead of url params or nested resource paths like /schema/cluster/{cluster_id}/keyspace/{keyspace}/table/{table}. But it is more succinct and we can revisit this once we have all our endpoints in place.
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
Description
This adds a GetSchema rpc to the vtadmin service, and an http wrapper at
/api/schema/{cluster_id}/{keyspace}/{table}
Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: