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.

Sign up for GitHub

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

Jump to bottom

Pass in cell to EnsureVSchema and GetOrCreateShard #5930

Merged
merged 7 commits into from
Apr 20, 2020
Merged

Pass in cell to EnsureVSchema and GetOrCreateShard #5930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e8497f7
Refactor EnsureVSchema to take in a cell
zmagg Apr 11, 2020
2a50906
Add cell to GetOrCreateShard to the test files too.
zmagg Apr 13, 2020
1724a71
Remove the nil check on CreateKeyspace to see if it lets you keep going
zmagg Apr 14, 2020
5c29a6b
Respond to code review. Add back in the nil check for CreateKeyspace.
zmagg Apr 15, 2020
ca85789
conditionally createkeyspace if it doesn't exist already.
zmagg Apr 16, 2020
72e75c1
ok try with the flaky test
zmagg Apr 16, 2020
f8edec4
If the keyspace already exists, don't try to create it again, it'll
zmagg Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions go/test/endtoend/recovery/recovery_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,19 @@ func VerifyQueriesUsingVtgate(t *testing.T, session *vtgateconn.VTGateSession, q

func RestoreTablet(t *testing.T, localCluster *cluster.LocalProcessCluster, tablet *cluster.Vttablet, restoreKSName string, shardName string, keyspaceName string, commonTabletArg []string) {
tablet.ValidateTabletRestart(t)
tm := time.Now().UTC()
tm.Format(time.RFC3339)
_, err := localCluster.VtctlProcess.ExecuteCommandWithOutput("CreateKeyspace",
"-keyspace_type=SNAPSHOT", "-base_keyspace="+keyspaceName,
"-snapshot_time", tm.Format(time.RFC3339), restoreKSName)
require.Nil(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should not be removed right?


replicaTabletArgs := commonTabletArg

Copy link
Member

Choose a reason for hiding this comment

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

@zmagg and I discussed this. It seems like this was a flaky test. The following change addresses that problem.

_, err := localCluster.VtctlProcess.ExecuteCommandWithOutput("GetKeyspace", restoreKSName)

if err != nil {
tm := time.Now().UTC()
tm.Format(time.RFC3339)
_, err := localCluster.VtctlProcess.ExecuteCommandWithOutput("CreateKeyspace",
"-keyspace_type=SNAPSHOT", "-base_keyspace="+keyspaceName,
"-snapshot_time", tm.Format(time.RFC3339), restoreKSName)
require.Nil(t, err)
}

if UseXb {
replicaTabletArgs = append(replicaTabletArgs, XbArgs...)
}
Expand Down
4 changes: 2 additions & 2 deletions go/vt/topo/shard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ func (ts *Server) CreateShard(ctx context.Context, keyspace, shard string) (err

// GetOrCreateShard will return the shard object, or create one if it doesn't
// already exist. Note the shard creation is protected by a keyspace Lock.
func (ts *Server) GetOrCreateShard(ctx context.Context, keyspace, shard string) (si *ShardInfo, err error) {
func (ts *Server) GetOrCreateShard(ctx context.Context, keyspace, shard, cell string) (si *ShardInfo, err error) {
log.Info("GetShard %s/%s", keyspace, shard)
si, err = ts.GetShard(ctx, keyspace, shard)
if !IsErrType(err, NoNode) {
Expand All @@ -340,7 +340,7 @@ func (ts *Server) GetOrCreateShard(ctx context.Context, keyspace, shard string)

// make sure a valid vschema has been loaded
log.Info("EnsureVSchema %s/%s", keyspace, shard)
if err = ts.EnsureVSchema(ctx, keyspace); err != nil {
if err = ts.EnsureVSchema(ctx, keyspace, []string{cell}); err != nil {
return nil, vterrors.Wrapf(err, "EnsureVSchema(%v) failed", keyspace)
}

Expand Down
4 changes: 2 additions & 2 deletions go/vt/topo/vschema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (ts *Server) GetVSchema(ctx context.Context, keyspace string) (*vschemapb.K
}

// EnsureVSchema makes sure that a vschema is present for this keyspace or creates a blank one if it is missing
func (ts *Server) EnsureVSchema(ctx context.Context, keyspace string) error {
func (ts *Server) EnsureVSchema(ctx context.Context, keyspace string, cells []string) error {
vschema, err := ts.GetVSchema(ctx, keyspace)
if err != nil && !IsErrType(err, NoNode) {
log.Info("error in getting vschema for keyspace %s: %v", keyspace, err)
Expand All @@ -89,7 +89,7 @@ func (ts *Server) EnsureVSchema(ctx context.Context, keyspace string) error {
}
}

err = ts.RebuildSrvVSchema(ctx, []string{} /* cells */)
Copy link
Member

Choose a reason for hiding this comment

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

What if we change EnsureVSchema to accept a cells list to rebuild, and we have each tablet rebuild only its own cell? Would that still have caused the problem you had?

Copy link
Contributor Author

@zmagg zmagg Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I like that a lot more. It still does the nice thing for single cell setups, with the benefit of being easier to reason about overall. I'll switch to that approach!

That would've been fine. The problem was that the tablet in the init loop wasn't just rebuilding its own cell it was rebuilding all the other cells.

err = ts.RebuildSrvVSchema(ctx, cells)
if err != nil {
log.Errorf("could not rebuild SrvVschema after creating keyspace: %v", err)
return err
Expand Down
5 changes: 3 additions & 2 deletions go/vt/topotools/shard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ func TestGetOrCreateShard(t *testing.T) {
ctx := context.Background()

// Set up topology.
ts := memorytopo.NewServer("test_cell")
cell := "test_cell"
ts := memorytopo.NewServer(cell)

// and do massive parallel GetOrCreateShard
keyspace := "test_keyspace"
Expand All @@ -119,7 +120,7 @@ func TestGetOrCreateShard(t *testing.T) {
for j := 0; j < 100; j++ {
index := rand.Intn(10)
shard := fmt.Sprintf("%v", index)
si, err := ts.GetOrCreateShard(ctx, keyspace, shard)
si, err := ts.GetOrCreateShard(ctx, keyspace, shard, cell)
if err != nil {
t.Errorf("GetOrCreateShard(%v, %v) failed: %v", i, shard, err)
}
Expand Down
7 changes: 6 additions & 1 deletion go/vt/vtctl/vtctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,7 +1620,12 @@ func commandCreateKeyspace(ctx context.Context, wr *wrangler.Wrangler, subFlags
}

if !*allowEmptyVSchema {
err = wr.TopoServer().EnsureVSchema(ctx, keyspace)
cells, err := wr.TopoServer().GetKnownCells(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I think we could also just pass an empty cells list to mean "all cells", since RebuildSrvVSchema already knows how to expand that by doing this same thing.

if err != nil {
return fmt.Errorf("GetKnownCells failed: %v", err)
}

err = wr.TopoServer().EnsureVSchema(ctx, keyspace, cells)
}

if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion go/vt/vttablet/tabletmanager/init_tablet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ func (agent *ActionAgent) InitTablet(port, gRPCPort int32) error {
var si *topo.ShardInfo
if err := agent.withRetry(ctx, "creating keyspace and shard", func() error {
var err error
si, err = agent.TopoServer.GetOrCreateShard(ctx, *initKeyspace, shard)
cell := agent.TabletAlias.GetCell()
si, err = agent.TopoServer.GetOrCreateShard(ctx, *initKeyspace, shard, cell)
return err
}); err != nil {
return vterrors.Wrap(err, "InitTablet cannot GetOrCreateShard shard")
Expand Down
2 changes: 1 addition & 1 deletion go/vt/wrangler/tablet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (wr *Wrangler) InitTablet(ctx context.Context, tablet *topodatapb.Tablet, a

if createShardAndKeyspace {
// create the parent keyspace and shard if needed
si, err = wr.ts.GetOrCreateShard(ctx, tablet.Keyspace, tablet.Shard)
si, err = wr.ts.GetOrCreateShard(ctx, tablet.Keyspace, tablet.Shard, tablet.Alias.GetCell())
} else {
si, err = wr.ts.GetShard(ctx, tablet.Keyspace, tablet.Shard)
if topo.IsErrType(err, topo.NoNode) {
Expand Down
4 changes: 2 additions & 2 deletions go/vt/wrangler/testlib/reparent_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestShardReplicationStatuses(t *testing.T) {
wr := wrangler.New(logutil.NewConsoleLogger(), ts, tmclient.NewTabletManagerClient())

// create shard and tablets
if _, err := ts.GetOrCreateShard(ctx, "test_keyspace", "0"); err != nil {
if _, err := ts.GetOrCreateShard(ctx, "test_keyspace", "0", "cell1"); err != nil {
t.Fatalf("GetOrCreateShard failed: %v", err)
}
master := NewFakeTablet(t, wr, "cell1", 1, topodatapb.TabletType_MASTER, nil)
Expand Down Expand Up @@ -108,7 +108,7 @@ func TestReparentTablet(t *testing.T) {
wr := wrangler.New(logutil.NewConsoleLogger(), ts, tmclient.NewTabletManagerClient())

// create shard and tablets
if _, err := ts.GetOrCreateShard(ctx, "test_keyspace", "0"); err != nil {
if _, err := ts.GetOrCreateShard(ctx, "test_keyspace", "0", "cell1"); err != nil {
t.Fatalf("CreateShard failed: %v", err)
}
master := NewFakeTablet(t, wr, "cell1", 1, topodatapb.TabletType_MASTER, nil)
Expand Down