InitTablet should not update master alias on shard record by deepthi · Pull Request #5316 · vitessio/vitess

deepthi · 2019-10-16T23:33:35Z

NOTE: This PR is targeting the reparent-refactor feature branch, not master.

vtctl InitTablet should not update shard's masterAlias. That will be done by the master tablet.
store masterTermStartTime on tablet record.

Signed-off-by: deepthi deepthi@planetscale.com

go/vt/wrangler/shard.go

…update shard master Signed-off-by: deepthi <deepthi@planetscale.com>

…arting with InitTablet Signed-off-by: deepthi <deepthi@planetscale.com>

applicable conditions vttablet InitTablet should check MasterTermStartTime and take over if necessary fix unit test setup to work with changes to InitTablet functions Signed-off-by: deepthi <deepthi@planetscale.com>

go/vt/wrangler/tablet.go

…n-zero Signed-off-by: deepthi <deepthi@planetscale.com>

proto/topodata.proto

go/vt/vttablet/tabletmanager/action_agent.go

go/vt/vttablet/tabletmanager/init_tablet.go

go/vt/vttablet/tabletmanager/rpc_actions.go

go/vt/wrangler/tablet.go

go/vt/wrangler/testlib/init_shard_master_test.go

enisoc · 2019-10-22T23:40:31Z

go/vt/wrangler/testlib/init_shard_master_test.go

 	master2 := NewFakeTablet(t, wr, "cell1", 1, topodatapb.TabletType_MASTER, db, ForceInitTablet())
-	if err := wr.InitShardMaster(ctx, master2.Tablet.Keyspace, master2.Tablet.Shard, master2.Tablet.Alias, false /*force*/, 10*time.Second); err == nil || !strings.Contains(err.Error(), "is not the only master in the shard") {
+
+	if err := wr.InitShardMaster(ctx, master2.Tablet.Keyspace, master2.Tablet.Shard, master2.Tablet.Alias, false /*force*/, 10*time.Second); err == nil || !strings.Contains(err.Error(), "is not the shard master") {


Why did this error change just now? I would have thought this test would have broken back when we made false masters demote themselves.

ShardSync doesn't run until we start the tablet. So this test didn't change when that was implemented.
Now that we have changed InitTablet to not set MasterAlias on the shard, that value is nil, so the error has changed.

I'm still worried this test is now no longer testing what it was meant to (detecting 2 masters in ISM). I think to keep the test an accurate simulation of 2 masters, we'd need to force the shard record to match one of the masters so ISM reaches the code that checks for 2 masters.

With that said, it may not be worth the effort to truly fix this test because 2 masters should not happen anymore anyway, and before long we want to do a whole overhaul of ISM. WDYT?

go/vt/wrangler/testlib/init_shard_master_test.go

Signed-off-by: deepthi <deepthi@planetscale.com>

go/vt/vttablet/tabletmanager/rpc_external_reparent.go

Signed-off-by: deepthi <deepthi@planetscale.com>

go/vt/vttablet/tabletmanager/rpc_external_reparent.go

go/vt/vttablet/tabletmanager/rpc_replication.go

go/vt/wrangler/testlib/external_reparent_test.go

Signed-off-by: deepthi <deepthi@planetscale.com>

go/vt/topotools/tablet.go

enisoc · 2019-10-25T20:06:36Z

go/vt/wrangler/testlib/init_shard_master_test.go

 	master2 := NewFakeTablet(t, wr, "cell1", 1, topodatapb.TabletType_MASTER, db, ForceInitTablet())
-	if err := wr.InitShardMaster(ctx, master2.Tablet.Keyspace, master2.Tablet.Shard, master2.Tablet.Alias, false /*force*/, 10*time.Second); err == nil || !strings.Contains(err.Error(), "is not the only master in the shard") {
+
+	if err := wr.InitShardMaster(ctx, master2.Tablet.Keyspace, master2.Tablet.Shard, master2.Tablet.Alias, false /*force*/, 10*time.Second); err == nil || !strings.Contains(err.Error(), "is not the shard master") {


I'm still worried this test is now no longer testing what it was meant to (detecting 2 masters in ISM). I think to keep the test an accurate simulation of 2 masters, we'd need to force the shard record to match one of the masters so ISM reaches the code that checks for 2 masters.

With that said, it may not be worth the effort to truly fix this test because 2 masters should not happen anymore anyway, and before long we want to do a whole overhaul of ISM. WDYT?

…that new tablet is returned only if there is no error Signed-off-by: deepthi <deepthi@planetscale.com>

enisoc

LGTM

deepthi requested a review from sougou as a code owner October 16, 2019 23:33

deepthi requested a review from enisoc October 16, 2019 23:33

sougou approved these changes Oct 17, 2019

View reviewed changes

enisoc reviewed Oct 17, 2019

View reviewed changes

go/vt/wrangler/shard.go Outdated Show resolved Hide resolved

deepthi changed the title ~~InitTablet should set MasterTermStartTime on shard record along with MasterAlias~~ WIP: InitTablet should not update master alias on shard record Oct 18, 2019

deepthi added 3 commits October 22, 2019 12:27

store master_term_start_time in tablet record. InitTablet should not …

57a6f48

…update shard master Signed-off-by: deepthi <deepthi@planetscale.com>

shard_sync should check for nil MasterAlias, which can happen when st…

e847d5e

…arting with InitTablet Signed-off-by: deepthi <deepthi@planetscale.com>

vtctl InitTablet should set tablet.MasterTermStartTime under all

67d2921

applicable conditions vttablet InitTablet should check MasterTermStartTime and take over if necessary fix unit test setup to work with changes to InitTablet functions Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi force-pushed the ds-init-tablet-master-ts branch from 2a2e630 to 67d2921 Compare October 22, 2019 19:28

deepthi changed the title ~~WIP: InitTablet should not update master alias on shard record~~ InitTablet should not update master alias on shard record Oct 22, 2019

deepthi commented Oct 22, 2019

View reviewed changes

go/vt/wrangler/tablet.go Outdated Show resolved Hide resolved

deepthi commented Oct 22, 2019

View reviewed changes

go/vt/wrangler/tablet.go Outdated Show resolved Hide resolved

InitTablet: only set MasterTermStartTime on tablet record if it is no…

69f2523

…n-zero Signed-off-by: deepthi <deepthi@planetscale.com>

enisoc reviewed Oct 22, 2019

View reviewed changes

set masterTermStartTime on tablet correctly whenever tablet type changes

8b6cca0

Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi commented Oct 24, 2019

View reviewed changes

go/vt/vttablet/tabletmanager/rpc_external_reparent.go Outdated Show resolved Hide resolved

fix tests

7006212

Signed-off-by: deepthi <deepthi@planetscale.com>

enisoc reviewed Oct 24, 2019

View reviewed changes

deepthi added 2 commits October 24, 2019 14:07

ChangeType should not update tablet if nothing changed

5e57501

Signed-off-by: deepthi <deepthi@planetscale.com>

changes from review

f2b8269

Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi force-pushed the ds-init-tablet-master-ts branch from 09e9f8b to f2b8269 Compare October 25, 2019 01:30

enisoc reviewed Oct 25, 2019

View reviewed changes

clean up ChangeType to avoid 2 topo calls, and to adhere to contract …

25e574d

…that new tablet is returned only if there is no error Signed-off-by: deepthi <deepthi@planetscale.com>

enisoc approved these changes Oct 25, 2019

View reviewed changes

deepthi merged commit d9fa8cd into vitessio:reparent-refactor Oct 25, 2019

deepthi deleted the ds-init-tablet-master-ts branch October 25, 2019 23:11

spark4 mentioned this pull request Nov 12, 2019

Serry deploy tinyspeck/vitess#140

Closed

spark4 mentioned this pull request Nov 22, 2019

Slack sync upstream 2019 11 09.r0 tinyspeck/vitess#142

Merged

rafael mentioned this pull request Dec 11, 2019

Slack sync upstream 2019 12 11.r0 tinyspeck/vitess#143

Merged

Conversation

deepthi commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enisoc Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

deepthi Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

enisoc Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enisoc Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

enisoc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deepthi commented Oct 16, 2019 •

edited

Loading