vttablet refactor phase 2 #6461

sougou · 2020-07-21T18:25:21Z

This is a the second part of the vttablet refactor. We essentially rewrite entire portions of the TabletManager, So, it may be easier to review the new functionality directly instead of looking at the diffs.

At a high level, this refactor achieves the following goals:

Healthcheck reporting was co-managed by TabletManager and TabletServer. This is now fully managed by TabletServer.
TabletServer's stateManager now has a consolidated set of variables that contribute to the serving state. These variables are now exposed through /debug/status.
All sub-components have to do their own retry if they fail to open.

Details

healthStreamer

This is a new component in TabletServer. It owns streaming of health info to subscribers like VTGate. It receives updates from stateManager whenever there is a material change in serving state.

replTracker

This component encapsulates the heartbeat reader, writer, and a new poller. It uses only one of these methods to report on replication health. If it's the poller, it gets invoked everytime a status check is performed. This is triggered from the stateManager.

stateManager

This component was introduced in a previous PR. It has now been enhanced to fetch replication info from replTracker, and feed it to healthStreamer. If the replication is unhealthy, stateManager also goes into a non-serving state. These substates are exported in /debug/status, and also there is a json export at the new path /debug/status_details.

replManager

This component watches over replication and fixes it as needed. This job was previously done by the replicationReporter.

tmState

This was functionality from state_change.go. There are now clear entry points for different state changes, like RefreshFromTopo, ChangeTabletType, etc.

deepthi

First pass

config/tablet/default.yaml

go/test/endtoend/cluster/mysqlctl_process.go

go/cmd/vttablet/status.go

go/test/endtoend/tabletmanager/tablet_health_test.go

go/vt/vttablet/endtoend/framework/client.go

go/vt/vttablet/endtoend/misc_test.go

go/vt/tableacl/tableacl.go

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Introduce a new Seconds type with explicit conversions to and from time.Duration. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

shutdown mysqls in parallel where possible. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

We saw another shard_sync panic in the tests due to a mismatch between tablet type and master term start time because they get fetched at different times instead of together. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

systay · 2020-07-24T07:34:08Z

go/vt/vttablet/tabletserver/health_streamer_test.go

+	assert.Equal(t, want, shr)
+}
+
+func testStream(t *testing.T, hs *healthStreamer) (<-chan *querypb.StreamHealthResponse, context.CancelFunc) {


t is not used in this method

systay · 2020-07-24T08:25:05Z

go/vt/vttablet/tabletmanager/rpc_replication.go

+		return err
+	}
+	if !si.HasMaster() {
+		return fmt.Errorf("no master tablet for shard %v/%v", tablet.Keyspace, tablet.Shard)


Any reason this file is using fmt.Error instead of vterrors?

I only moved this code from somewhere else. All these older functions are still using fmt.Errorf.

I could not see any test file for this.

There are end2end tests for this. We should ideally have unit tests also, but it's legacy code.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deepthi

Nice work. A few minor nits.

go/vt/vttablet/tabletmanager/replmanager.go

go/vt/vttablet/tabletmanager/replmanager_test.go

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

harshit-gangal · 2020-07-25T23:05:05Z

go/vt/vttablet/tabletserver/health_streamer.go

+	})
+}
+
+func (hs *healthStreamer) ApppendDetails(details []*kv) []*kv {


I do not see any test for this method

There's no framework to test the /debug/status pages right now. But I tested it manually though.

harshit-gangal · 2020-07-25T23:20:35Z

go/vt/vttablet/tabletmanager/replmanager.go

+	if rm.markerFile == "" {
+		return
+	}
+	if stopped {
+		if file, err := os.Create(rm.markerFile); err == nil {
+			file.Close()
+		}
+	} else {
+		os.Remove(rm.markerFile)
+	}
+}


When markerFile be empty? If it is only for test, can we change the test to not have empty markerFile?

markerFile can be empty if the mysql is unmanaged. There are end2end tests for testing the overall behavior. I've written some unit tests to cover the rest of the corner cases that are not covered by the e2e tests.

harshit-gangal · 2020-07-25T23:24:44Z

go/vt/vttablet/tabletmanager/rpc_replication.go

+		return err
+	}
+	if !si.HasMaster() {
+		return fmt.Errorf("no master tablet for shard %v/%v", tablet.Keyspace, tablet.Shard)


I could not see any test file for this.

harshit-gangal · 2020-07-25T23:30:37Z

go/vt/vttablet/tabletmanager/tm_state.go

+	if ts.tm.UpdateStream != nil {
+		if topo.IsRunningUpdateStream(ts.tablet.Type) {
+			ts.tm.UpdateStream.Enable()
+		} else {
+			ts.tm.UpdateStream.Disable()
+		}
+	}
+
+	if ts.tm.VREngine != nil {
+		if ts.tablet.Type == topodatapb.TabletType_MASTER {
+			ts.tm.VREngine.Open(ts.tm.BatchCtx)
+		} else {
+			ts.tm.VREngine.Close()
+		}
+	}


UpdateStream and VREngine are not set in the test. See if can be added in test tm initialize

The unit test framework doesn't support these. But there are many end2end tests for this.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

deepthi · 2020-07-27T19:38:28Z

Fixes #6131 #6345

sougou requested review from systay, deepthi, harshit-gangal and enisoc July 21, 2020 18:25

sougou marked this pull request as ready for review July 22, 2020 05:39

deepthi mentioned this pull request Jul 22, 2020

removed error check while verifying the replication #6462

Merged

deepthi reviewed Jul 22, 2020

View reviewed changes

sougou force-pushed the ss-ts2-health branch from b716b97 to 4703d99 Compare July 22, 2020 16:57

sougou added 22 commits July 22, 2020 21:25

vttablet: stateManager tests WIP

f5b5e12

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

tabletserver: create new ReplTracker skeleton

b1affae

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: move hc flags to tabletenv.TabletConfig

9ba72cc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: type safe Seconds instead of float64

c9fac6a

Introduce a new Seconds type with explicit conversions to and from time.Duration. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: enable_replication_reporter -> tabletenv

925768d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: saving work. need mysqld in tabletserver

64c2075

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: add mysqld to tabletserver

4d7ff07

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: ReplTracker functional

3ce3440

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: switch to use replTracker

2b4ed04

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: tests for ReplTracker

4915ff6

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: new healthStreamer

35dadde

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: wire-up most of healthStreamer

13ce1d8

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: streamHealth wired up

1e2b0f6

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: status page uses healthStreamer

91d3063

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: VREngine retries if Open fails

ff9290e

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

tm: preparing to delete healthcheck

b565677

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: trying a different heathcheck

5122818

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: delete old broken health tests

3bb412a

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: make BACKUP StateNotConnected

06fc13d

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: fix tabletserver repl health & tests

6cbba23

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: relpManager initial code

07c4c95

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: fix tabletmanager tests

3d720ae

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou added 9 commits July 22, 2020 21:25

end2end: speed up tests

37432f1

shutdown mysqls in parallel where possible. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

end2end: remove most InitTablet and CreateDB calls

94603f5

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: tracker closed on non-serving

4f7a59f

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

end2end: InitTablet removed: reparent & cellalias

3f2247a

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: VREngine retry tests

9e1bb43

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: test for alsoAllow

c8d1076

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: tm_state tests

5afbc90

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet: add polish

1555def

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

vttablet refactor: address review comments

dce6346

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the ss-ts2-health branch from 4703d99 to dce6346 Compare July 23, 2020 04:25

vttablet: fix another possible race condition

6da60f7

We saw another shard_sync panic in the tests due to a mismatch between tablet type and master term start time because they get fetched at different times instead of together. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

systay reviewed Jul 24, 2020

View reviewed changes

vttablet: address systay review comments

c699293

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou mentioned this pull request Jul 24, 2020

RFC: vttablet breaking changes #6345

Closed

deepthi approved these changes Jul 25, 2020

View reviewed changes

go/vt/vttablet/tabletmanager/replmanager.go Show resolved Hide resolved

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

vttablet: more review comments

7cd9ba8

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

harshit-gangal reviewed Jul 25, 2020

View reviewed changes

harshit-gangal approved these changes Jul 25, 2020

View reviewed changes

vttablet: address race condition in test

6a1745f

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou merged commit f4269bc into vitessio:master Jul 26, 2020

shlomi-noach deleted the ss-ts2-health branch July 26, 2020 08:07

deepthi mentioned this pull request Jul 27, 2020

RFC: Change tabletmanager model #6131

Closed

deepthi added this to the v7.0 milestone Jul 27, 2020

ameetkotian mentioned this pull request Aug 20, 2020

Slack vitess 2020.08.19.r0 tinyspeck/vitess#180

Merged

dweitzman mentioned this pull request Nov 2, 2020

Regression: statsTabletType not initialized in 7.0.3 #6988

Closed

deepthi mentioned this pull request Apr 6, 2022

Resharding: SwitchWrites leaves source masters in an unhealthy state #6822

Closed

GuptaManan100 mentioned this pull request Dec 6, 2022

Tracking CheckMySQL and Query Engine history #11885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vttablet refactor phase 2 #6461

vttablet refactor phase 2 #6461

sougou commented Jul 21, 2020 •

edited

Loading

deepthi left a comment

systay Jul 24, 2020

systay Jul 24, 2020

sougou Jul 24, 2020

harshit-gangal Jul 25, 2020

sougou Jul 26, 2020

deepthi left a comment •

edited

Loading

harshit-gangal Jul 25, 2020

sougou Jul 26, 2020

harshit-gangal Jul 25, 2020

sougou Jul 26, 2020

harshit-gangal Jul 25, 2020

harshit-gangal Jul 25, 2020

sougou Jul 26, 2020

deepthi commented Jul 27, 2020

vttablet refactor phase 2 #6461

vttablet refactor phase 2 #6461

Conversation

sougou commented Jul 21, 2020 • edited Loading

Details

healthStreamer

replTracker

stateManager

replManager

tmState

deepthi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi commented Jul 27, 2020

sougou commented Jul 21, 2020 •

edited

Loading

deepthi left a comment •

edited

Loading