Healthcheck: new simpler implementation by deepthi · Pull Request #6155 · vitessio/vitess

deepthi · 2020-05-07T05:45:21Z

Fixes #5750
There is now a new gateway implementation called tabletgateway which uses the new healthcheck.
Setting vtgate's flag -gateway_implementation=tabletgateway will use the new gateway with the new healthcheck.
The existing healthcheck has been renamed to LegacyHealthCheck. It continues to be used by all other current users.

go/vt/discovery/healthcheck.go

sougou · 2020-05-15T23:17:40Z

go/vt/discovery/healthcheck.go

Close looks unused.

go/vt/discovery/healthcheck.go

sougou · 2020-05-15T23:36:28Z

go/vt/discovery/healthcheck.go

I think this function is unreachable (because Gateway does not have a Close).
But I see it being covered in tests.

Should Gateway export the function? Something to think about.

vtgate seems to be missing a servenv.OnClose. That would be the logical place to call Close on gateway / healthcheck.

Gateway already has a Close through the QueryService interface.

go/vt/discovery/healthcheck.go

go/vt/discovery/tablet_health.go

go/vt/discovery/healthcheck.go

sougou · 2020-05-16T00:08:10Z

This is from my initial skimming. I'm still working on the review.

sougou

I glanced at topo watcher and stats cache. I assume they didn't change much.

go/vt/discovery/healthcheck.go

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

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

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

…of tabletStatsCache, refactor to encapsulate more behavior that belongs in healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

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

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

…he. rename tabletStats to tabletHealth Signed-off-by: deepthi <deepthi@planetscale.com>

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

…le tabletType change in healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

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

…ecorder interface. Move cellsAliases cache into TopologyWatcher Signed-off-by: deepthi <deepthi@planetscale.com>

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

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

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

implement healthy tablet sorting by replica lag, notify buffer when master changes. Signed-off-by: deepthi <deepthi@planetscale.com>

… get basic unit test working Signed-off-by: deepthi <deepthi@planetscale.com>

… TabletHealth struct that is returned to gateway. Unit tests Signed-off-by: deepthi <deepthi@planetscale.com>

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

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

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

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

…ch results in incomplete error information Signed-off-by: deepthi <deepthi@planetscale.com>

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

…HealthCheck, move checkConn and finalizeConn to tabletHealthCheck, rename deleteConn -> deleteTablet Signed-off-by: deepthi <deepthi@planetscale.com>

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

sougou

This is looking great. Couple more changes.

go/vt/discovery/healthcheck.go

go/vt/discovery/tablet_health.go

Signed-off-by: Andres Taylor <andres@planetscale.com>

harshit-gangal

Executor test should use new healthCheck. This will unblock from adding new funtionality (replication tx, reserve connection) using this new healthcheck.

This also means that with legacyhealthcheck additional functionality will not work. So, should we not go ahead with breaking change and remove the legacy heath check all together?

harshit-gangal · 2020-05-25T11:47:34Z

go/cmd/vtgate/vtgate.go

+		_ = vtg.Gateway().Close(context.Background())
+		if legacyHealthCheck != nil {
+			_ = legacyHealthCheck.Close()
+		}


nit: can change the function call
"_ = vtg.Gateway().Close(context.Background())" -> "vtg.Gateway().Close(context.Background())"
"_ = legacyHealthCheck.Close()" -> "legacyHealthCheck.Close()"

Ignoring the return value produces warnings.

this is still ignored :)

I personally think it's clearer using the underscore to mark that we are aware of the returned value, but we are ignoring it. Makes it clear that it's not by mistake

go/vt/vtgate/executor.go

harshit-gangal · 2020-05-25T13:37:16Z

go/vt/vtgate/executor.go

-				if !ts.Serving {
-					state = "NOT_SERVING"
+		if *GatewayImplementation == GatewayImplementationDiscovery {
+			stats := e.scatterConn.GetLegacyHealthCheckCacheStatus()


For this method call the scatterConn struct contains legacyHealthCheck discovery.LegacyHealthCheck. The HealthCheck and LegacyHealthCheck can have an interface implementation that can return the rows back. This can avoid keeping legacyhealthcheck inside scatterconn

scatterConn already had a reference to healthcheck in the legacy implementation. We are not changing that in this iteration because we want to minimize how much we touch the legacy implementation. However, I do see that we can simplify this so that there is less code repetition in executor.

actually.. unless we make LegacyTabletsCacheStatusList and TabletsCacheStatusList implement the same interface, I don't see how to avoid the duplicated code.

I agree, I leave it up to you.

My thinking is that leaving in ugly if-else blocks will force us to clean it up soon :)

go/vt/discovery/healthcheck_test.go

go/vt/discovery/healthcheck.go

go/vt/discovery/legacy_healthcheck.go

…abletHealthCheck with TabletHealth, incorporate review comments to make code more readable, start using modern techniques in tests Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi · 2020-05-25T21:45:32Z

Executor test should use new healthCheck. This will unblock from adding new funtionality (replication tx, reserve connection) using this new healthcheck.

We can do that, but I would rather not have that block this PR. It can be done separately.

This also means that with legacyhealthcheck additional functionality will not work. So, should we not go ahead with breaking change and remove the legacy heath check all together?

We discussed this. There are a few reasons to keep the legacy health check.

If things break, there will be a fallback
There are other things besides vtgate that use the legacy healthcheck. We need to clean those up to go to a pure discovery (vs. health) model.

We did decide to make the new healthcheck the default, so I have committed that change.

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

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

…ad of thread-unsafe healthByAlias from cacheStatusMap Signed-off-by: deepthi <deepthi@planetscale.com>

…eck also, so move it to replicationlag.go Signed-off-by: deepthi <deepthi@planetscale.com>

sougou · 2020-05-27T02:08:27Z

🎉

deepthi requested a review from sougou as a code owner May 7, 2020 05:45

sougou reviewed May 16, 2020

View reviewed changes

sougou reviewed May 18, 2020

View reviewed changes

go/vt/discovery/healthcheck.go Outdated Show resolved Hide resolved

deepthi added 26 commits May 19, 2020 22:21

healthcheck: rename healthcheck and associated structs to legacy

8dd5132

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

healthcheck: move more code to legacy, and create stubs for new healt…

f543f21

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

healthcheck: remove unused structs, compactify new healthcheck functi…

27ec840

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

healthcheck: move tabletStats into separate file, implement cell map …

b29b745

…of tabletStatsCache, refactor to encapsulate more behavior that belongs in healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: implement topo watch, tablet_stats_cache test, stub new …

9e3adee

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

healthcheck: remove unnecessary flag, fix some TODOs

d4cb214

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

healthcheck: simplify healthcheck struct by deleting tablet_stats_cac…

21b9f74

…he. rename tabletStats to tabletHealth Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: if cells_to_watch is empty, operate on just localCell

8e2b518

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

healthcheck: delete commented code

5808934

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

healthcheck: use tabletHealth.mu to serialize access to members. hand…

3d95319

…le tabletType change in healthcheck Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: move shuffleTablets back to gateway

84f21ab

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

healthcheck: Move topoWatch back into TopologyWatcher and use TabletR…

30c6f47

…ecorder interface. Move cellsAliases cache into TopologyWatcher Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: remove references to TabletToMapKey in new topo watcher

b0ce1c5

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

healthcheck: notify buffer when failover ends, rename entries -> heal…

1abc4f4

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

healthcheck: rename old tests to legacy

8a020f3

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

healthcheck: move loadTablets from TopologyWatcher to HealthCheckImpl,

543dd23

implement healthy tablet sorting by replica lag, notify buffer when master changes. Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: fold healthCheckConn into TabletHealth, fix CacheStatus,…

1c511af

… get basic unit test working Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: rename TabletHealth->tabletHealthCheck, create a simpler…

fe9bdd4

… TabletHealth struct that is returned to gateway. Unit tests Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: HealthCheck cache in vtgate debug/status page

741c012

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

healthcheck: refactor stream into smaller funcs, use channel for unit…

94e597f

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

healthcheck: ScatterConn should return the correct cache status based…

4b85e4b

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

healthcheck: topology_watcher unit tests

b2f8c89

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

healthcheck: fix stats and healthy, endtoend test

8946b06

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

healthcheck: endtoend test for scatter_conn and buffer

2b95b25

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

healthcheck: lock hc.mu before accessing members

3c89878

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

healthcheck: unit tests for GetHealthyTabletStats

08250b8

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

deepthi added 4 commits May 19, 2020 22:21

healthcheck: update protobuf generated sources, fix shadowing bug whi…

00d137e

…ch results in incomplete error information Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: cell alias test, add new tests to config.json

c4fab25

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

healthcheck: cell alias unit test

97a77a4

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

healthcheck: remove HealthCheck interface, rename HealthCheckImpl -> …

e0fb77e

…HealthCheck, move checkConn and finalizeConn to tabletHealthCheck, rename deleteConn -> deleteTablet Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi force-pushed the ds-new-healthcheck branch from 10475cf to e0fb77e Compare May 20, 2020 05:21

deepthi added 3 commits May 21, 2020 21:34

healthcheck: use channel to notify buffer of new master

08deeac

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

healthcheck: close subscriber goroutine when healthcheck is closed

edd4066

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

healthcheck: fix endtoend setup to use correct gateway

45b9b1d

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

sougou reviewed May 25, 2020

View reviewed changes

addressed review comments

3f172ca

Signed-off-by: Andres Taylor <andres@planetscale.com>

harshit-gangal reviewed May 25, 2020

View reviewed changes

healthcheck: remove mu from tabletHealthCheck, replace more uses of t…

cb276c9

…abletHealthCheck with TabletHealth, incorporate review comments to make code more readable, start using modern techniques in tests Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi added 4 commits May 25, 2020 15:09

healthcheck: make new tabletgateway the default, deprecate discoveryg…

f56e82b

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

healthcheck: fix vtexplain test to use legacy gateway

a49e079

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

healthcheck: fix race condition by using thread-safe healthData inste…

d9e420b

…ad of thread-unsafe healthByAlias from cacheStatusMap Signed-off-by: deepthi <deepthi@planetscale.com>

healthcheck: legacy_replication_lag_algorithm is used by new healthch…

3343c21

…eck also, so move it to replicationlag.go Signed-off-by: deepthi <deepthi@planetscale.com>

harshit-gangal approved these changes May 26, 2020

View reviewed changes

systay approved these changes May 26, 2020

View reviewed changes

sougou approved these changes May 27, 2020

View reviewed changes

sougou merged commit bcbee47 into vitessio:master May 27, 2020

deepthi deleted the ds-new-healthcheck branch July 6, 2020 19:00

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

ameetkotian mentioned this pull request Aug 20, 2020

Slack vitess 2020.08.19.r0 tinyspeck/vitess#180

Merged

harshit-gangal restored the ds-new-healthcheck branch May 7, 2021 17:13

frouioui mentioned this pull request Mar 9, 2022

Delete discovery gateway #9500

Merged

3 tasks

GuptaManan100 mentioned this pull request Dec 6, 2022

Tracking CheckMySQL and Query Engine history #11885

Closed

Conversation

deepthi commented May 7, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sougou commented May 16, 2020

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harshit-gangal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deepthi commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sougou commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deepthi May 25, 2020 •

edited

Loading

deepthi commented May 25, 2020 •

edited

Loading