-
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] vschemas api endpoints #7625
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>
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.
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; |
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.
My one tiny + totally non-blocking comment is that I would have expected the field to be called vschema
(no snake). 🐍
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, 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!
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.
Ah that makes sense. The generated TypeScript types also turn out as VSchema
which I agree looks best. 😎 Thanks for the explanation!
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 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?).
Merging based on review from @doeg |
Description
This PR adds the
GetVSchema
andGetVSchemas
endpoints to vtadmin-api.Related Issue(s)
Checklist
Deployment Notes
I've canaried this internally and looks good.
Impacted Areas in Vitess
Components that this PR will affect: