[vtctld] Migrate `ApplyVSchema` to `VtctldServer` · Pull Request #8113 · vitessio/vitess

ghost · 2021-05-12T18:46:24Z

@ajm188 am i on the right track here? if so, i'll add tests.
context: https://vitess.slack.com/archives/C0PQY0PTK/p1620754861183700

The goal here is to add the ApplyVSchema api call to the grpc-vtctld service.

Signed-off-by: Alex Charis alex.charis@shopify.com

Checklist

Tests were added or are not required
Documentation was added or is not required

ghost · 2021-05-12T18:47:03Z

CC: @alexrs

ajm188

This is definitely on the right track. In addition to the testing (there are many examples of how to use the helpers in server_test.go), you'll also need to add a cli entrypoint, and I think that's where we should include the sql-mode/json-mode behavior of the original vtctl command

ajm188 · 2021-05-12T18:56:42Z

go/vt/vtctl/grpcvtctldserver/server.go

nit: please try to keep this file sorted. it makes scanning it much easier as it grows ever larger

ajm188 · 2021-05-12T18:56:50Z

proto/vtctldata.proto

ditto on sorting

ajm188 · 2021-05-12T18:56:57Z

proto/vtctlservice.proto

ditto on sorting

ajm188 · 2021-05-12T18:57:42Z

proto/vtctldata.proto

This should include a skip_rebuild option to match the original implementation

ajm188 · 2021-05-12T19:00:34Z

go/vt/vtctl/grpcvtctldserver/server.go

looks like you're missing the "keyspace existence" check

ajm188 · 2021-05-12T19:16:40Z

proto/vtctldata.proto

I wonder if it will be useful to include the final, saved vschema here (after reading it back out of the topo, similar to how ChangeTabletType works

and similar to how the current vtctlclient command works

ghost · 2021-05-14T17:52:53Z

still needs tests and cli entrypoint, but i wanted to get eyes on this again before i take it out of draft.
and i wanted to see if the failing check here in github was a flake or if it failed again.

ajm188

This is looking great, just some minor feedback that I noticed now, but I think you're good to take it out of draft and work on testing/cli

ajm188 · 2021-05-15T12:33:30Z

go/vt/vtctl/grpcvtctldserver/server.go

this should be if topo.IsErrType(err, topo.NoNode)

ajm188 · 2021-05-15T12:35:55Z

go/vt/vtctl/grpcvtctldserver/server.go

we should wrap these errors to give some extra context of which step failed

ghost · 2021-06-01T17:56:47Z

i found some more time to circle back on this.
it still needs tests, but i got the CLI done.
i'll rebase to fix the proto conflicts.

ghost · 2021-06-01T17:57:18Z

go/cmd/vtctldclient/internal/command/vschemas.go

happy to validate here as well. wdyt?

imo we should validate as much server-side as possible, that way the implementation can be shared between vtctldserver and the legacy vtctl commands

ghost · 2021-06-01T18:16:37Z

not sure why these are being marked as conflicting, i just rebased and recompiled protos. i'll take another look.

ajm188 · 2021-06-02T02:07:36Z

not sure why these are being marked as conflicting, i just rebased and recompiled protos. i'll take another look.

Unfortunately I think I had just merged another vtctld rpc PR, so I am the cause of your merge conflicts this time 😞 sorry about that!

ghost · 2021-06-02T14:40:21Z

yep, another pull and rebase (and proto compile) seems to have gotten rid of the conflicts. thanks

ajm188

This is looking great. I think we just need to move that sqlparsing/topotools logic from the client to the server and we're good to go. Really sorry that didn't click for me earlier

ajm188 · 2021-06-02T17:06:14Z

go/cmd/vtctldclient/internal/command/vschemas.go

Apologies for not catching this during my first pass, but I think this entire chunk of logic needs to move to the server, as the only way to access this logic is by invoking the client binary (in general my view is that the client commands should do the bare minimum to convert CLI args into a proto message and then invoke the corresponding RPC).

So, I think what we want is:

message ApplyVSchemaRequest { // [ other fields elided ] vschema.Keyspace vschema = N; // this is the result of `--json` mode on the client, and we apply the vschema blindly string sql = N+1; // this is --sql mode, and we do the parsing + topotools call on the server side }

interesting, i can definitely imagine someone calling the new api with only an sql string rather than a vschema object. so i can see where you're coming from.
i think i liked the idea of keeping the flag validation on the client side up above the call to FinishedParsing.
(though generally i do agree with what you say below about doing as much validation on the server side as possible.)

i do need to keep the file reading parts on the client side though, obviously. so it's not going to be a complete cut+paste job. but i'm definitely happy to shift the bulk of the logic up to the server side.

ajm188 · 2021-06-02T18:03:01Z

go/vt/vtctl/grpcvtctldserver/server.go

I would make this an vterrors.Wrapf(vtrpc.FAILED_PRECONDITION, ...)

not totally sure what you meant here.
Wrap/Wrapf don't take an error code like failed precondition.
New and Errorf do take a code, but those are for making a new error, whereas here i already have an error.

ajm188 · 2021-06-02T18:05:00Z

go/vt/vtctl/grpcvtctldserver/server.go

nit: wrong error string (probably copy-paste from above?)

ajm188 · 2021-06-02T18:06:50Z

go/cmd/vtctldclient/internal/command/vschemas.go

imo we should validate as much server-side as possible, that way the implementation can be shared between vtctldserver and the legacy vtctl commands

ajm188 · 2021-06-02T18:12:25Z

go/cmd/vtctldclient/internal/command/vschemas.go

A net-new thing (that you could not have known!) I'm trying to introduce is:

Set DisableFlagsInUseLine: true on every command. This will change the help from {{ .Use }} [flags] to just {{ .Use }} (which is better, because you include the actual named flags in the Use string).

Include a .Short help text, which in most cases I just copy from the old vtctl command.

ghost · 2021-06-03T15:22:17Z

(in general my view is that the client commands should do the bare minimum to convert CLI args into a proto message and then invoke the corresponding RPC).

~~do you want the dry run logic on the server side as well, then?~~
turns out that's the only way to do it

ghost · 2021-06-03T18:34:53Z

with the shift of that logic, i've got to expand the testing a bit i think.

Signed-off-by: Alex Charis <alex.charis@shopify.com>

keyspace existence check, and return updated vschema, handle skip_rebuild Signed-off-by: Alex Charis <alex.charis@shopify.com>

Signed-off-by: Alex Charis <alex.charis@shopify.com>

ajm188

some minor changes, then lgtm! thanks for doing this!

ajm188 · 2021-06-05T15:48:55Z

go/vt/vtctl/grpcvtctldserver/server_test.go

 	vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata"
 	vtctlservicepb "vitess.io/vitess/go/vt/proto/vtctlservice"
-	"vitess.io/vitess/go/vt/proto/vttime"
+	vttime "vitess.io/vitess/go/vt/proto/vttime"


This doesn't need an alias

ajm188 · 2021-06-07T14:34:46Z

go/vt/vtctl/grpcvtctldserver/server_test.go

 		ts        *topo.Server
 		setup     func(ts *topo.Server) error
-		req       *vtctldata.DeleteCellsAliasRequest
+		req       *vtctldatapb.DeleteCellsAliasRequest


thanks for catching this one, i must have missed this in another change

ajm188

whoops, meant to include these comments in my last review

ajm188 · 2021-06-07T14:36:15Z

go/cmd/vtctldclient/internal/command/vschemas.go


 func init() {
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.VSchema, "vschema", "", "VSchema")
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.VSchemaFile, "vschema_file", "", "VSchema File")


we're using dashes instead of underscores in the new client

ajm188 · 2021-06-07T14:36:23Z

go/cmd/vtctldclient/internal/command/vschemas.go

+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.VSchema, "vschema", "", "VSchema")
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.VSchemaFile, "vschema_file", "", "VSchema File")
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.SQL, "sql", "", "A VSchema DDL SQL statement, e.g. `alter table t add vindex hash(id)`")
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.SQLFile, "sql_file", "", "A file containing VSchema DDL SQL")


ajm188 · 2021-06-07T14:36:32Z

go/cmd/vtctldclient/internal/command/vschemas.go

+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.SQL, "sql", "", "A VSchema DDL SQL statement, e.g. `alter table t add vindex hash(id)`")
+	ApplyVSchema.Flags().StringVar(&applyVSchemaOptions.SQLFile, "sql_file", "", "A file containing VSchema DDL SQL")
+	ApplyVSchema.Flags().BoolVar(&applyVSchemaOptions.DryRun, "dry-run", false, "If set, do not save the altered vschema, simply echo to console.")
+	ApplyVSchema.Flags().BoolVar(&applyVSchemaOptions.SkipRebuild, "skip_rebuild", false, "If set, do no rebuild the SrvSchema objects.")


Signed-off-by: Alex Charis <alex.charis@shopify.com>

ghost · 2021-06-07T15:30:27Z

@ajm188 alright, tests passing. are we good to go?

systay · 2021-06-08T14:46:48Z

Since we are creating the release notes automatically from PRs, I was wondering if we could tweak the title a bit? Should the WIP prefix still be there?

ajm188 · 2021-06-08T14:50:23Z

Nope! I updated it just now, let me know if that's good for the release notes tooling or if I should tweak it differently

ghost · 2021-06-08T15:41:05Z

thanks!

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 reviewed May 12, 2021

View reviewed changes

ajm188 reviewed May 15, 2021

View reviewed changes

ghost commented Jun 1, 2021

View reviewed changes

ghost force-pushed the vtctld-applyvschema branch from 74a6c7a to 0c93793 Compare June 1, 2021 18:13

ghost marked this pull request as ready for review June 1, 2021 18:15

ghost requested a review from doeg as a code owner June 1, 2021 18:15

ghost force-pushed the vtctld-applyvschema branch from 0c93793 to 72e4d67 Compare June 2, 2021 14:38

ajm188 reviewed Jun 2, 2021

View reviewed changes

derekperkins added the Component: Cluster management label Jun 3, 2021

Alex Charis added 9 commits June 4, 2021 13:22

implement ApplyVSchema in grpc-vtctld

cb21654

Signed-off-by: Alex Charis <alex.charis@shopify.com>

added skip_rebuild and v_schema to message types

335e880

keyspace existence check, and return updated vschema, handle skip_rebuild Signed-off-by: Alex Charis <alex.charis@shopify.com>

added command line interface

7ce57de

Signed-off-by: Alex Charis <alex.charis@shopify.com>

recompiled protos after rebase

1852224

Signed-off-by: Alex Charis <alex.charis@shopify.com>

rebuild protos and client_gen

640af1e

Signed-off-by: Alex Charis <alex.charis@shopify.com>

added a test

5c85d70

Signed-off-by: Alex Charis <alex.charis@shopify.com>

allow sql_mode or json_mode on the server-side

6632d48

Signed-off-by: Alex Charis <alex.charis@shopify.com>

add more tests; which compile, but do not pass

2b4ab4c

Signed-off-by: Alex Charis <alex.charis@shopify.com>

tests passing

785c3b6

Signed-off-by: Alex Charis <alex.charis@shopify.com>

ghost force-pushed the vtctld-applyvschema branch from 0a44f79 to 785c3b6 Compare June 4, 2021 15:50

ghost added the Type: Enhancement Logical improvement (somewhere between a bug and feature) label Jun 4, 2021

ghost requested a review from ajm188 June 4, 2021 16:28

remove the single letter versions of these flags

4f325fe

Signed-off-by: Alex Charis <alex.charis@shopify.com>

ajm188 approved these changes Jun 7, 2021

View reviewed changes

ajm188 reviewed Jun 7, 2021

View reviewed changes

no vttime alias; underscore -> hyphen

d63a4ce

Signed-off-by: Alex Charis <alex.charis@shopify.com>

ajm188 merged commit 7dfbc60 into vitessio:main Jun 7, 2021

ghost deleted the vtctld-applyvschema branch June 7, 2021 18:43

ajm188 changed the title ~~WIP: implement ApplyVSchema in grpc-vtctld~~ [vtctld] Migrate ApplyVSchema to VtctldServer Jun 8, 2021

ajm188 changed the title ~~[vtctld] Migrate ApplyVSchema to VtctldServer~~ [vtctld] Migrate ApplyVSchema to VtctldServer Jun 8, 2021

ajm188 mentioned this pull request Jun 9, 2021

Fix data races in grpcvtctldserver tests #8299

Closed

ajm188 added a commit to tinyspeck/vitess that referenced this pull request Jul 23, 2021

Merge pull request vitessio#8113 from Shopify/vtctld-applyvschema

6d561fa

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 mentioned this pull request Jul 23, 2021

[do not merge] Slack vitess v10.0.2.vtctld tinyspeck/vitess#231

Closed

8 tasks

vmogilev mentioned this pull request May 23, 2022

[do not merge] Slack vitess v11.0.4.vtctld.pre tinyspeck/vitess#247

Open

33 tasks

Conversation

ghost commented May 12, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ghost commented May 12, 2021

Uh oh!

ajm188 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 14, 2021

Uh oh!

ajm188 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 1, 2021

Uh oh!

ajm188 commented Jun 2, 2021

Uh oh!

ghost commented Jun 2, 2021

Uh oh!

ajm188 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 3, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 3, 2021

Uh oh!

ajm188 left a comment

ghost commented May 12, 2021 •

edited by ghost

Loading

ghost commented Jun 3, 2021 •

edited by ghost

Loading

systay commented Jun 8, 2021 •

edited

Loading