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

merged 7 commits into from
Apr 20, 2020

Conversation

zmagg
Copy link
Contributor

@zmagg zmagg commented Mar 17, 2020 •
edited
Loading

Description

Modifies EnsureVSchema so that we always pass in a cell and it rebuilds SrvVSchema only in the provided cell. This removes its auto-rebuild of all cells automatically when you're creating a new shard or keyspace.

Background

During a setup of a new region, we had a misconfigured topology and spinning up a new shard caused us to corrupt all cells in the topology, just as part of booting up a new tablet. For safety reasons, it seems better to decouple spinning up a new shard from rebuilding SrvVSchema on all cells. As talked about in person a few weeks ago--we'll keep the rebuild for the simple case where there's only one cell.

Signed-off-by: Maggie Zhou mzhou@slack-corp.com

@zmagg zmagg requested a review from sougou as a code owner March 17, 2020 06:18
@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch 2 times, most recently from 6109866 to 29b8e36 Compare March 19, 2020 17:56
enisoc
enisoc reviewed Mar 26, 2020
View reviewed changes
go/vt/topo/vschema.go
@@ -82,10 +83,21 @@ 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.

@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch 8 times, most recently from a70acbf to 97268cb Compare April 15, 2020 18:01
rafael
rafael reviewed Apr 15, 2020
View reviewed changes
Copy link
Member

@rafael rafael left a comment

Choose a reason for hiding this comment

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

Maggie this is looking great. I added minor comments.

go/test/endtoend/recovery/recovery_util.go
@@ -54,7 +54,6 @@ func RestoreTablet(t *testing.T, localCluster *cluster.LocalProcessCluster, tabl
_, 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?

go/vt/topo/vschema.go Outdated
if err != nil {
log.Errorf("could not rebuild SrvVschema after creating keyspace: %v", err)
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

nit - remove extra new line.

@zmagg zmagg changed the title Skip RebuildSrvVschema in InitTablet for multi-cell configurations Pass in cell to EnsureVSchema and GetOrCreateShard Apr 15, 2020
enisoc
enisoc approved these changes Apr 15, 2020
View reviewed changes
Copy link
Member

@enisoc enisoc left a comment

Choose a reason for hiding this comment

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

LGTM with one optional suggestion.

go/vt/vtctl/vtctl.go
@@ -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.

@zmagg zmagg closed this Apr 15, 2020
@zmagg zmagg reopened this Apr 15, 2020
@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch from f927513 to c2deff9 Compare April 16, 2020 06:16
@zmagg
Refactor EnsureVSchema to take in a cell
e8497f7 
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
Add cell to GetOrCreateShard to the test files too.
2a50906 
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
Remove the nil check on CreateKeyspace to see if it lets you keep going
1724a71 
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
Respond to code review. Add back in the nil check for CreateKeyspace.
5c29a6b 
Remove empty line.

Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
conditionally createkeyspace if it doesn't exist already.
ca85789 
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg
ok try with the flaky test
72e75c1 
Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch from 4f5ded2 to 72e75c1 Compare April 16, 2020 07:00
@deepthi
Copy link
Member

deepthi commented Apr 16, 2020 

local.sharded_recovery                  	FAIL (3 tries)

This test hasn't been flaky. Does it pass for you locally?

@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch from c0f1f4e to aadd900 Compare April 16, 2020 21:58
@zmagg
If the keyspace already exists, don't try to create it again, it'll
f8edec4 
throw an error

Signed-off-by: Maggie Zhou <mzhou@slack-corp.com>
@zmagg zmagg force-pushed the zmagg_rebuild_srv_schema_no_multicell branch from aadd900 to f8edec4 Compare April 16, 2020 23:04
rafael
rafael reviewed Apr 20, 2020
View reviewed changes
go/test/endtoend/recovery/recovery_util.go
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.

@rafael rafael merged commit 924824b into vitessio:master Apr 20, 2020
@rafael rafael deleted the zmagg_rebuild_srv_schema_no_multicell branch April 20, 2020 17:57
ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Apr 28, 2020
@rafael @ajm188
Merge pull request vitessio#5930 from tinyspeck/zmagg_rebuild_srv_sch…
94d336d 
…ema_no_multicell

Pass in cell to EnsureVSchema and GetOrCreateShard
@ajm188 ajm188 mentioned this pull request Apr 28, 2020
Merged
@enisoc enisoc mentioned this pull request Jun 5, 2020
Merged
@ajm188 ajm188 mentioned this pull request Jun 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enisoc enisoc enisoc approved these changes

@rafael rafael rafael approved these changes

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zmagg @deepthi @rafael @enisoc