Migrate `vtctlclient InitShardMaster` => `vtctldclient InitShardPrimary` #7220

ajm188 · 2020-12-22T21:52:56Z

Description

This implements the InitShardMaster command in the VtctldServer, renamed as InitShardPrimary to take advantage of the easy opportunity to rename without backwards compatibility considerations.

Some other decisions made in this PR to call out:

I did no refactor or variable renames on the function, because I didn't want to add complications to the initial migration.
The old version had no tests, including endtoend tests, so I added a couple simple cases. Any further advice on testing is welcome.
Important - I included the well-known Duration type to handle the --wait-replicas-timeout flag, which requires inclusion of the standard proto definitions that ship with protoc. These usually (and by default I think) end up in /usr/local/include, so I've taken the (possibly naive!) simple step of updating make proto to add that include path. I'd like to talk about ideas about how to ensure this doesn't break existing developer's workflows, and whether this is portable enough. Like, we could, for example, call this good enough and add a note to the proto readme to ensure you have these files in the right path, or we could make the tooling more dynamic.

Related Issue(s)

Checklist

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

Deployment Notes

None

Impacted Areas in Vitess

Components that this PR will affect:

Query Serving
VReplication
Cluster Management
Build

ajm188 · 2020-12-23T01:04:44Z

proto/vtctlservice.proto

+  // WARNING: This could cause data loss on an already replicating shard.
+  // PlannedReparentShard or EmergencyReparentShard should be used in those
+  // cases instead.
+  rpc InitShardPrimary(vtctldata.InitShardPrimaryRequest) returns (vtctldata.InitShardPrimaryResponse) {};


I'm genuinely unsure what the right API here is. Is all we want the stream of updates? Maybe this is a case where we actually want just a stream logutil.Event response 🤔

I don't know either :) What you have is fine for now and we can fix forward if we decide otherwise.

doeg

The golang parts look good to me! Thank you for doing the rename (and adding tests too, so nice!)

You'll fur sure want Planetscale signoff on the rest, of course. 🎉

deepthi · 2021-01-06T00:15:42Z

I included the well-known Duration type to handle the --wait-replicas-timeout flag, which requires inclusion of the standard proto definitions that ship with protoc. These usually (and by default I think) end up in /usr/local/include, so I've taken the (possibly naive!) simple step of updating make proto to add that include path. I'd like to talk about ideas about how to ensure this doesn't break existing developer's workflows, and whether this is portable enough. Like, we could, for example, call this good enough and add a note to the proto readme to ensure you have these files in the right path, or we could make the tooling more dynamic.

In my local environment, the standard protos are under vitess.io/vitess/dist/vt-protoc-3.6.1/include. That is where bootstrap installs them.

ajm188 · 2021-01-06T00:47:10Z

Working updated make proto:

❯ make proto
Tue Jan 5 19:44:17 EST 2021: Installing minimal dependencies
skipping protoc install. remove /Users/amason/work/vitess/dist/vt-protoc-3.6.1 to force re-install.
/usr/local/bin/etcd
etcd already installed
skipping k3s install. remove /Users/amason/work/vitess/dist/k3s to force re-install.

bootstrap finished - run 'make build' to compile
Tue Jan 5 19:44:57 EST 2021: Compiling proto definitions

deepthi

LGTM

…mples There are no functional changes in this commit, just a pure port with the most minor changes to ensure identical behavior (like the callback logger). I also updated our `proto` target to assume a standard protoc installation (which installs a stdlib of well-known types into /usr/local/include). We can decide if this is okay to make a requirement, or if we should try to make this more portable in PR review. I also updated the local example to use the vtctldclient port, and went through the full example with no issues. Signed-off-by: Andrew Mason <amason@slack-corp.com>

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

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

ajm188 · 2021-01-14T02:13:18Z

go/vt/proto/automationservice/automationservice.pb.go

@@ -12,7 +12,6 @@ import (
 	grpc "google.golang.org/grpc"
 	codes "google.golang.org/grpc/codes"
 	status "google.golang.org/grpc/status"
-


it looks like the new goimports style conflicts with how protoc-gen-go generates files

Fixes for this in 602dd69 (git hook) and 67ae893 (linter config)

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

rohit-nayak-ps · 2021-01-14T15:01:31Z

@ajm188, I reran a flaky test that had failed and now all tests are passing. Is this ready to be merged?

ajm188 · 2021-01-14T15:25:42Z

Yep should be good to go!

ajm188 requested review from deepthi and rohit-nayak-ps December 22, 2020 21:52

ajm188 requested review from doeg and sougou as code owners December 22, 2020 21:52

ajm188 force-pushed the am_init_shard_primary branch from 40b0f19 to 3e6942d Compare December 23, 2020 01:00

ajm188 commented Dec 23, 2020

View reviewed changes

doeg approved these changes Dec 29, 2020

View reviewed changes

ajm188 force-pushed the am_init_shard_primary branch from 3e6942d to 283db12 Compare January 6, 2021 00:18

deepthi approved these changes Jan 12, 2021

View reviewed changes

ajm188 added 4 commits January 13, 2021 21:10

Add endtoend tests for basic ISM functionality

02d2b27

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

Rewrite to share code between wrangler and vtctld server

672bf95

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

Make proto target depend on minimaltools, so we have protos installed…

99b507c

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

ajm188 force-pushed the am_init_shard_primary branch from f1343d4 to 99b507c Compare January 14, 2021 02:11

ajm188 commented Jan 14, 2021

View reviewed changes

ajm188 added 3 commits January 13, 2021 21:16

Exclude protoc-gen-go generated files from goimports checks

602dd69

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

Exclude protoc-gen-go generated files from the linter goimports check

67ae893

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

Rerun goimports

c7b1360

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

rohit-nayak-ps merged commit d0e4fff into vitessio:master Jan 14, 2021

ajm188 deleted the am_init_shard_primary branch January 14, 2021 15:39

askdba added this to the v9.0 milestone Jan 19, 2021

ajm188 added Component: Cluster management Type: Feature Request labels May 23, 2021

ajm188 added this to In progress in Vtctld Service via automation May 23, 2021

ajm188 moved this from In progress to Done in Vtctld Service May 23, 2021

eseokoh mentioned this pull request May 31, 2021

Release Note of v9.0.0 includes changes of v10.0 #8212

Closed

ajm188 mentioned this pull request Jul 12, 2021

slack vitess v10.pre tinyspeck/vitess#228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `vtctlclient InitShardMaster` => `vtctldclient InitShardPrimary` #7220

Migrate `vtctlclient InitShardMaster` => `vtctldclient InitShardPrimary` #7220

ajm188 commented Dec 22, 2020

ajm188 Dec 23, 2020

deepthi Jan 12, 2021

doeg left a comment

deepthi commented Jan 6, 2021

ajm188 commented Jan 6, 2021

deepthi left a comment

ajm188 Jan 14, 2021

ajm188 Jan 14, 2021

rohit-nayak-ps commented Jan 14, 2021

ajm188 commented Jan 14, 2021

Migrate vtctlclient InitShardMaster => vtctldclient InitShardPrimary #7220

Migrate vtctlclient InitShardMaster => vtctldclient InitShardPrimary #7220

Conversation

ajm188 commented Dec 22, 2020

Description

Related Issue(s)

Checklist

Deployment Notes

Impacted Areas in Vitess

ajm188 Dec 23, 2020

Choose a reason for hiding this comment

deepthi Jan 12, 2021

Choose a reason for hiding this comment

doeg left a comment

Choose a reason for hiding this comment

deepthi commented Jan 6, 2021

ajm188 commented Jan 6, 2021

deepthi left a comment

Choose a reason for hiding this comment

ajm188 Jan 14, 2021

Choose a reason for hiding this comment

ajm188 Jan 14, 2021

Choose a reason for hiding this comment

rohit-nayak-ps commented Jan 14, 2021

ajm188 commented Jan 14, 2021

Migrate `vtctlclient InitShardMaster` => `vtctldclient InitShardPrimary` #7220

Migrate `vtctlclient InitShardMaster` => `vtctldclient InitShardPrimary` #7220