healthcheck: refactor to use less locking by sougou · Pull Request #3919 · vitessio/vitess

sougou · 2018-05-11T01:03:53Z

This change gets rid of all locks in hcc. Instead we use the approach
of "sharing by communicating". Whenever there is a change in state,
hcc communicates the change to hc, which then performs the necessary
updates and handling.

Now that hc updates are trivial, the lock has been changed to
a simple Mutex, which is more efficient that RWMutex.

Also, instead of looping through every stream, this change
makes every stream goroutine perform its own timeout check.

demmer

After an initial (non-exhaustive) pass on this change one high level reaction is that it seems potentially confusing to have two TabletStats structs for each downstream tablet, one in the tabletHealth data structure and another for each healthCheckConn.

I wonder if it would be possible to remove the one from healthCheckConn since that's no longer used for making routing decisions and then we can accomplish the goals of the refactor without the duplication.

demmer · 2018-05-23T04:06:09Z

go/vt/discovery/healthcheck.go

It would be helpful to have some comments indicating what each of these fields does. In particular I think this used to be called streamCancelFunc which made it (somewhat) clearer what it did.

Also it's not clear from an initial read what the relationship is between the tabletStats in this struct vs the one in healthCheckConn.

IMO renaming to something like latestTabletStats would make it clearer.

cancelFunc wasn't renamed. It was moved from hcc into here. The streamCancelFunc that was in hcc is now a local variable within the goroutine that runs the healthcheck.

Explained the relationship between hcc and th in the comments.

Renamed tabletStats->latestTabletStats

demmer · 2018-05-23T04:19:41Z

go/vt/discovery/healthcheck.go

As we found out the hard way, under certain circumstances this could take a long time. so I think it's cleaner to update the stats to mark the connection as not serving before doing the connection close which is how I did it before.

Good catch. Done.

demmer · 2018-05-23T04:23:26Z

go/vt/discovery/healthcheck.go

I think it would be clearer if serving was declared inside the watcher goroutine and not outside.

It's correct as written but since they're only used in the context of the watcher I think that's clearer.

demmer · 2018-05-23T04:27:09Z

go/vt/discovery/healthcheck.go

timedout is set here inside the watcher goroutine but is then read outside in the main thread.

This seems like a data race to me since there's no synchronization.

Nice catch. I've changed timedout to use atomic operations.

demmer · 2018-05-23T04:41:37Z

go/vt/discovery/healthcheck.go

Although very very unlikely in practice, it seems like bad things would happen if the watcher goroutine got stalled and the servingStatus channel does actually fill up. Instead of 5 slots would it make more sense to just have one and then select here in the unlikely event it's blocked?

Makes sense.

demmer · 2018-05-23T04:47:53Z

go/vt/discovery/healthcheck.go

Instead of calling copy() on the way in, it seems more defensive to copy it here.

Doing the copy here will reduce some duplication, but it may not separate the concerns correctly. It will introduce a requirement that healthCheckConn should not change the contents of ts while updateHealth is executing.

But I could be convinced otherwise.

sougou

Most comments addressed, and some responses inline.

On the high level approach of tabletHealth vs healthCheckConn: The separation was needed to simplify the locking scheme. In the new world, healthCheckConn is internal to the checkConn goroutine. For each tabletHealth, a checkConn is launched, which is responsible for providing health updates. So, tabletHealth only cares about the goroutine, and knows nothing of healthCheckConn.

I've added a TODO for moving healthCheckConn and the checkConn to their own file.

sougou · 2018-05-26T04:00:24Z

go/vt/discovery/healthcheck.go

cancelFunc wasn't renamed. It was moved from hcc into here. The streamCancelFunc that was in hcc is now a local variable within the goroutine that runs the healthcheck.

Explained the relationship between hcc and th in the comments.

Renamed tabletStats->latestTabletStats

sougou · 2018-05-26T04:43:26Z

go/vt/discovery/healthcheck.go

Doing the copy here will reduce some duplication, but it may not separate the concerns correctly. It will introduce a requirement that healthCheckConn should not change the contents of ts while updateHealth is executing.

But I could be convinced otherwise.

sougou · 2018-05-26T04:43:40Z

go/vt/discovery/healthcheck.go

Good catch. Done.

sougou · 2018-05-26T04:43:53Z

go/vt/discovery/healthcheck.go

sougou · 2018-05-26T04:51:32Z

go/vt/discovery/healthcheck.go

Makes sense.

sougou · 2018-05-26T04:55:38Z

go/vt/discovery/healthcheck.go

Nice catch. I've changed timedout to use atomic operations.

sougou · 2018-05-26T16:19:31Z

I had to move the serving assignment out of the goroutine. It introduced a data race.

Instead of looping through every stream, this change makes every stream goroutine perform its own timeout check. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

This change gets rid of all locks in hcc. Instead we use the approach of "sharing by communicating". Whenever there is a change in state, hcc communicates the change to hc, which then performs the necessary updates and handling. Also, now that hc updates are trivial, the lock has been changed to a simple Mutex, which is more efficient that RWMutex. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

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

It turns out that moving the serving assignment within the goroutine introduces a data race. I've moved it back out. Also found another incidental data race: did you know that functions like t.Fatalf should not be called from goroutines? Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

demmer

I made another pass through this after it being in the queue for a long time...

In general I think this refactor is a great step in the right direction, but there are a number of suggestions to improve things even more and one possibly dangerous bug lurking in here that I think should be fixed.

go/vt/discovery/healthcheck.go

demmer · 2018-10-05T14:16:53Z

go/vt/discovery/healthcheck.go

-	if conn == nil {
-		var err error
-		conn, err = tabletconn.GetDialer()(hcc.tabletStats.Tablet, grpcclient.FailFast(true))
+	if hcc.conn == nil {


This was already an issue before this change, but it's hard to reason about why some methods are attached to the healthCheckConn (taking the HealthCheckImpl as a parameter), while others are attached to the impl and take the conn as a parameter.

My intuition is that this would all be a lot clearer if it was consistent -- any functionality that refers to a single connection (checkConn, finalizeConn, stream, etc) would be defined on the conn struct, while functionality related to the overall data structure (UpdateHealth, AddTablet, etc) would be defined on the containing impl struct.

That would also help when you split out healthcheck_conn into a separate file.

I just tried making this change, but I encountered a readability problem: checkConn calls hc.connsWG.Done(), which belongs to HealthCheckImpl. This would be confusing if we moved this to a different file. Also, checkConn references multiple other things in hc. So, we'll probably need to do a deeper refactor to clean up this mess.

But returns are diminishing.

go/vt/discovery/healthcheck.go

demmer · 2018-10-05T14:28:57Z

go/vt/discovery/healthcheck.go

+		// later.
+		timedout := sync2.NewAtomicBool(false)
+		serving := hcc.tabletStats.Serving
+		go func() {


I think this may need to bump up the overall threads running wg and then bump down when the goroutine exits?

If you see below, after the stream call, we call streamCancel which will ensure this goroutine will eventually exit. There's no need for this goroutine to exit before HC is closed. We just have to make sure we don't leak them.

But I found a deeper problem. An inherent race between Close and all the other AddTablet, etc. I've now changed Close to set addrToHealth to nil as a guard to ensure nobody accepts more changes after HC is closed.

go/vt/discovery/healthcheck.go

Also found a race between Close and other calls that modify state. That case is also fixed. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou requested a review from demmer May 11, 2018 01:04

demmer reviewed May 23, 2018

View reviewed changes

sougou commented May 26, 2018

View reviewed changes

sougou added 4 commits September 22, 2018 16:43

healthcheck: refactor stream timeout code

18e1bde

Instead of looping through every stream, this change makes every stream goroutine perform its own timeout check. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

healthcheck: address review comments.

2debc94

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

demmer requested changes Oct 5, 2018

View reviewed changes

healthcheck: address comments

de45de6

Also found a race between Close and other calls that modify state. That case is also fixed. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

demmer approved these changes Nov 18, 2018

View reviewed changes

sougou merged commit 606738d into vitessio:master Nov 18, 2018

sougou deleted the healthcheck branch November 18, 2018 17:48

rafael mentioned this pull request Dec 3, 2018

Slack vitess 2018 12 3.r0 tinyspeck/vitess#118

Merged

snyk-bot mentioned this pull request Jul 14, 2021

[Snyk] Security upgrade protractor from 3.3.0 to 5.0.0 SNiTEBoBy/vitess-gh#11

Open

Conversation

sougou commented May 11, 2018

Uh oh!

demmer 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!

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!

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!

sougou 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sougou commented May 26, 2018

Uh oh!

demmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants