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

[vtadmin] vschemas api endpoints #7625

Merged
merged 7 commits into from Mar 8, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Mar 6, 2021

Description

This PR adds the GetVSchema and GetVSchemas endpoints to vtadmin-api.

Related Issue(s)

Checklist

  • Should this PR be backported? no
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

I've canaried this internally and looks good.

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • 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>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Copy link
Contributor

@doeg doeg left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you so much!!

Cluster cluster = 1;
// Name is the name of the keyspace this VSchema is for.
string name = 2;
vschema.Keyspace v_schema = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

My one tiny + totally non-blocking comment is that I would have expected the field to be called vschema (no snake). 🐍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and this is my biggest issue with the protobuf/protoc toolchain, is I have no idea how to force it to name things in the generated code a particular way. If I don't snake case, then the field name in the generated Go code would be Vschema, when I want (because I would expect it to be) VSchema; the snake case forces the Go naming that I want, but it makes the typescript+json naming completely unintuitive. As far as I know, there's no way to have both (I'll do some more googling though!), and I'm naturally biased toward picking the "nice for Go" side because it's where I spend most of my time, but I could be convinced otherwise!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that makes sense. The generated TypeScript types also turn out as VSchema which I agree looks best. 😎 Thanks for the explanation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at this some more, and if I use just the natively available json_name extension:

diff --git a/proto/vtadmin.proto b/proto/vtadmin.proto
index 91a2939aa..7bf137793 100644
--- a/proto/vtadmin.proto
+++ b/proto/vtadmin.proto
@@ -106,7 +106,7 @@ message VSchema {
     Cluster cluster = 1;
     // Name is the name of the keyspace this VSchema is for.
     string name = 2;
-    vschema.Keyspace v_schema = 3;
+    vschema.Keyspace v_schema = 3 [json_name="vschema,omitempty"];
 }
 
 // Vtctld represents information about a single Vtctld host.
diff --git a/go/vt/proto/vtadmin/vtadmin.pb.go b/go/vt/proto/vtadmin/vtadmin.pb.go
index c897510cf..bfe1bb5a7 100644
--- a/go/vt/proto/vtadmin/vtadmin.pb.go
+++ b/go/vt/proto/vtadmin/vtadmin.pb.go
@@ -313,7 +313,7 @@ type VSchema struct {
        Cluster *Cluster `protobuf:"bytes,1,opt,name=cluster,proto3" json:"cluster,omitempty"`
        // Name is the name of the keyspace this VSchema is for.
        Name                 string            `protobuf:"bytes,2,opt,name=name,proto3" json:"name,o
mitempty"`
-       VSchema              *vschema.Keyspace `protobuf:"bytes,3,opt,name=v_schema,json=vSchema,pro
to3" json:"v_schema,omitempty"`
+       VSchema              *vschema.Keyspace `protobuf:"bytes,3,opt,name=v_schema,json=vschema,omi
tempty,proto3" json:"v_schema,omitempty"`
        XXX_NoUnkeyedLiteral struct{}          `json:"-"`
        XXX_unrecognized     []byte            `json:"-"`
        XXX_sizecache        int32             `json:"-"`

(Note that the json field within the protobuf struct tag changes, but the actual json struct tag does not).

However, apparently if we start relying on extensions only available using gogoproto's types, then this should work (stackoverflow post with example), but if I'm reading the takeaways from this discussion correctly, I think that's something vitess isn't quite comfortable committing to (yet?).

@rafael
Copy link
Member

rafael commented Mar 8, 2021

Merging based on review from @doeg

@rafael rafael merged commit 2c9063e into vitessio:master Mar 8, 2021
@askdba askdba added the Component: VTAdmin VTadmin interface label Mar 10, 2021
@askdba askdba added this to the v10.0 milestone Mar 10, 2021
@doeg doeg added this to In progress in VTAdmin via automation Mar 16, 2021
@doeg doeg moved this from In progress to Done in VTAdmin Mar 16, 2021
@ajm188 ajm188 deleted the am_vtadmin_vschemas branch May 29, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTAdmin VTadmin interface
Projects
Development

Successfully merging this pull request may close these issues.

Add GetVSchema(s) endpoints to vtadmin-api
4 participants