Skip to content

Commit

Permalink
tm revamp: remove unnecessary runHealthCheck
Browse files Browse the repository at this point in the history
ChangeType was invoking a runHealthCheck at the end, but this is
unnecessary because the refreshTablet call already runs it. I found
it while changing tests that were using the old TER to use the
new ChangeType calls.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
  • Loading branch information
sougou committed Jun 3, 2020
1 parent 7e09a38 commit c033d50
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 28 deletions.
2 changes: 1 addition & 1 deletion go/vt/vtcombo/tablet_map.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func CreateTablet(ctx context.Context, ts *topo.Server, cell string, uid uint32,
}
agent := tabletmanager.NewComboActionAgent(ctx, ts, alias, int32(8000+uid), int32(9000+uid), controller, dbcfgs, mysqld, keyspace, shard, dbname, strings.ToLower(initTabletType.String()))
if tabletType == topodatapb.TabletType_MASTER {
if err := agent.TabletExternallyReparented(ctx, ""); err != nil {
if err := agent.ChangeType(ctx, topodatapb.TabletType_MASTER); err != nil {
return fmt.Errorf("TabletExternallyReparented failed on master %v: %v", topoproto.TabletAliasString(alias), err)
}
}
Expand Down
7 changes: 3 additions & 4 deletions go/vt/vttablet/tabletmanager/healthcheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -717,13 +717,12 @@ func TestStateChangeImmediateHealthBroadcast(t *testing.T) {
t.Fatal(err)
}

// Run TER to turn us into a proper master, wait for it to finish.
// Change to master.
agent.HealthReporter.(*fakeHealthCheck).reportReplicationDelay = 19 * time.Second
if err := agent.TabletExternallyReparented(ctx, "unused_id"); err != nil {
if err := agent.ChangeType(ctx, topodatapb.TabletType_MASTER); err != nil {
t.Fatalf("TabletExternallyReparented failed: %v", err)
}
<-agent.finalizeReparentCtx.Done()
// It is not enough to wait for finalizeReparentCtx to be done, we have to wait for shard_sync to finish
// Wait for shard_sync to finish
startTime := time.Now()
for {
if time.Since(startTime) > 10*time.Second /* timeout */ {
Expand Down
4 changes: 1 addition & 3 deletions go/vt/vttablet/tabletmanager/rpc_actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ func (agent *ActionAgent) changeTypeLocked(ctx context.Context, tabletType topod
}

// let's update our internal state (stop query service and other things)
// refreshTablet will runHealthCheck.
if err := agent.refreshTablet(ctx, "ChangeType"); err != nil {
return err
}
Expand All @@ -102,9 +103,6 @@ func (agent *ActionAgent) changeTypeLocked(ctx context.Context, tabletType topod
if err := agent.fixSemiSyncAndReplication(agent.Tablet().Type); err != nil {
return vterrors.Wrap(err, "fixSemiSyncAndReplication failed, may not ack correctly")
}

// and re-run health check
agent.runHealthCheckLocked()
return nil
}

Expand Down
23 changes: 3 additions & 20 deletions go/vt/vttablet/tabletmanager/rpc_replication.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,14 @@ import (
"strings"
"time"

"vitess.io/vitess/go/vt/logutil"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/topotools"
"vitess.io/vitess/go/vt/vterrors"

"golang.org/x/net/context"

"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/topo/topoproto"
"vitess.io/vitess/go/vt/vterrors"

replicationdatapb "vitess.io/vitess/go/vt/proto/replicationdata"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
Expand Down Expand Up @@ -227,21 +224,7 @@ func (agent *ActionAgent) InitMaster(ctx context.Context) (string, error) {
return "", err
}

// This section is similar to changeTypeLocked except that we don't
// invoke runHealthCheck. This is because the tabletserver cannot come
// up until the database is created, which happens after the replicas
// are pointed at this new master.
startTime := time.Now()
_, err = topotools.ChangeType(ctx, agent.TopoServer, agent.TabletAlias, topodatapb.TabletType_MASTER, logutil.TimeToProto(startTime))
if err != nil {
return "", err
}
// We only update agent's masterTermStartTime if we were able to update the topo.
// This ensures that in case of a failure, we are never in a situation where the
// tablet's timestamp is ahead of the topo's timestamp.
agent.setMasterTermStartTime(startTime)
// and refresh our state
if err := agent.refreshTablet(ctx, "InitMaster"); err != nil {
if err := agent.changeTypeLocked(ctx, topodatapb.TabletType_MASTER); err != nil {
return "", err
}
return mysql.EncodePosition(pos), nil
Expand Down

0 comments on commit c033d50

Please sign in to comment.