Dedicated poolDialer logic for VTOrc, throttler by shlomi-noach · Pull Request #15562 · vitessio/vitess

shlomi-noach · 2024-03-25T07:18:41Z

Description

Did I say #15560 was the final alternative? This is final 2.0. So #15560 ran into a couple problems, seemingly locking. I'm investing far too much time in this problem, so this PR, as per @harshit-gangal 's suggestion, keeps the existing pools completely untouched, and adds a new mechanism for the non-standard pool dialers (ie vtorc and throttler). These two non-standard dialers only have a single tmc hence a single cached connection. For any error whatsoever received in FullStatus or in CheckThrottler, the connection gets invalidated (it's too aggressive, but better than existing situation).

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-03-25T07:18:43Z

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-03-25T09:46:22Z

Added some unit testing.

codecov · 2024-03-25T09:56:15Z

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 65.77%. Comparing base (4f24560) to head (5528591).
Report is 4 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/grpctmclient/client.go	92.68%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15562      +/-   ##
==========================================
+ Coverage   65.73%   65.77%   +0.03%     
==========================================
  Files        1560     1561       +1     
  Lines      194595   194755     +160     
==========================================
+ Hits       127921   128102     +181     
+ Misses      66674    66653      -21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbussink

Let's go for this one yeah, and see separately how we can fix up those other cases. I suspect we're probably good enough for a single multiplexed connection in the end anyway?

shlomi-noach · 2024-03-25T10:49:09Z

I suspect we're probably good enough for a single multiplexed connection in the end anyway?

I believe so. Certainly for the throttler and for vtorc. For the rest, more experimentation needed.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

GuptaManan100

I like this implementation!

GuptaManan100 · 2024-03-26T05:14:00Z

go/vt/vttablet/grpctmclient/client.go

+		invalidator := func(err error) {
+			client.mu.Lock()
+			defer client.mu.Unlock()
+			m[addr].cc.Close()
+			delete(m, addr)
+		}


Just one question. We take in the error here, but don't check for the error to be a connection error, or even if it's nil. Won't this cause us to invalidate the connections after every call?

we call invalidator only when there is an error, passing of error should be removed

Good catch on not checking for err == nil! Updated.
Re: connection error, we've already established last week that it's very difficult to distinguish between gRPC errors, TCP errors, vitess errors, MySQL errors etc., because the call stack is such that errors are wrapped up as SQLErr including gRPC ones. We therefore invalidate on any error.

Mind you that in this PR we only create an invalidation function for FullStatus and for CheckThrottler, where SQL errors are highly unexpected, as opposed to ExecuteFetch* where such errors are trivial.

Yep, makes sense!

I further updated to reflect @harshit-gangal 's comment. Calling invalidator() only on error, and now not even passing the error argument.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal · 2024-03-26T05:18:46Z

go/vt/vttablet/grpctmclient/client.go

+	if dialPoolGroup != DialPoolGroupDefault {
+		client.mu.Lock()
+		defer client.mu.Unlock()
+		if client.rpcDialPoolMap == nil {
+			client.rpcDialPoolMap = make(map[DialPoolGroup]addrTmcMap)
+		}


nit: default and non-default looks like a different code path,
can this be changed to a switch condition and separate method calls for readability?

nit: default and non-default looks like a different code path,

I've now split into dialPool (near identical the original function) and dialDedicatedPool (new behavior). Can you please take another look?

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-03-26T05:34:10Z

go/vt/vttablet/grpctmclient/client.go

 	var c tabletmanagerservicepb.TabletManagerClient
+	var invalidator invalidatorFunc
 	var err error
 	if poolDialer, ok := client.dialer.(poolDialer); ok {


I wonder in what scenario is the dialer not a poolDialer?

harshit-gangal · 2024-03-26T05:39:02Z

go/vt/vttablet/grpctmclient/client.go

+		if invalidator != nil {
+			invalidator()
+		}
 		return nil, err


nit: this will not be nil

It could be nil, if this condition fails:

if poolDialer, ok := client.dialer.(poolDialer); ok

Now, I'm not sure how it could possibly fail, but as golang goes, I created var invalidator invalidatorFunc and it could be nil.

If you like, I can pre-assign var invalidator to an empty function? That way it will never be nil.

This kinda returns to this comment: #14979 (comment)

harshit-gangal · 2024-03-26T05:39:13Z

go/vt/vttablet/grpctmclient/client.go


 	response, err := c.CheckThrottler(ctx, req)
 	if err != nil {
+		if invalidator != nil {


shlomi-noach · 2024-03-26T06:11:09Z

Merging for now. I think we still need a refactor to the pooDialer code, and that's where we can address casting, nil, naming, etc. issues.

shlomi-noach · 2024-03-26T06:11:26Z

This is a connection leak bugfix, and as such, should be backported to v19.

… (#15567) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

GuptaManan100 and others added 3 commits March 19, 2024 14:49

test: add failing test for full status connection pooling

92c3641

Signed-off-by: Manan Gupta <manan@planetscale.com>

Merge remote-tracking branch 'origin/main' into tmc-pool-dialer-5

54276e4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

separate logic for non default dial groups

db6e3c3

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Bug Component: VTOrc Vitess Orchestrator integration Component: TabletManager Component: Throttler labels Mar 25, 2024

shlomi-noach requested review from GuptaManan100, ajm188, deepthi and rohit-nayak-ps as code owners March 25, 2024 07:18

shlomi-noach removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Mar 25, 2024

github-actions bot added this to the v20.0.0 milestone Mar 25, 2024

This was referenced Mar 25, 2024

WIP: TabletManagerClient real pool dialer #15541

Closed

Fix for FullStatus gRPC connection pooling #15520

Closed

shlomi-noach changed the title ~~Tmc pool dialer 5~~ Dedicated poolDialer logic for VTOrc, throttler Mar 25, 2024

shlomi-noach removed the NeedsIssue A linked issue is missing for this Pull Request label Mar 25, 2024

shlomi-noach requested a review from a team March 25, 2024 07:36

shlomi-noach added 2 commits March 25, 2024 11:38

declare itnermediate variable

033a2da

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

add unit tests

f01fcc4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink approved these changes Mar 25, 2024

View reviewed changes

shlomi-noach mentioned this pull request Mar 25, 2024

WIP: dedicated pool dialers to vtorc and to throttler functions, no change in "concurrency" #15560

Closed

5 tasks

shlomi-noach requested a review from harshit-gangal March 25, 2024 11:11

more unit testing

813456a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

GuptaManan100 approved these changes Mar 26, 2024

View reviewed changes

GuptaManan100 reviewed Mar 26, 2024

View reviewed changes

only invalidate on error

5d4303b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal reviewed Mar 26, 2024

View reviewed changes

only invalidate on error, remove err argument

e3f8c74

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal approved these changes Mar 26, 2024

View reviewed changes

split logic into dialPool and dialDedicatedPool

5528591

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach commented Mar 26, 2024

View reviewed changes

harshit-gangal reviewed Mar 26, 2024

View reviewed changes

harshit-gangal approved these changes Mar 26, 2024

View reviewed changes

shlomi-noach added the Backport to: release-19.0 label Mar 26, 2024

shlomi-noach merged commit 90c0057 into vitessio:main Mar 26, 2024

shlomi-noach deleted the tmc-pool-dialer-5 branch March 26, 2024 06:14

vitess-bot bot mentioned this pull request Mar 26, 2024

[release-19.0] Dedicated poolDialer logic for VTOrc, throttler (#15562) #15567

Merged

shlomi-noach mentioned this pull request Mar 26, 2024

Bug Report: leaking tablet client connections in poolDialer #15563

Closed

GuptaManan100 mentioned this pull request Apr 19, 2024

Bug Report: vtorc Client received GoAway with error code ENHANCE_YOUR_CALM and debug data equal to ASCII "too_many_pings" #15726

Closed

Conversation

shlomi-noach commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Mar 25, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

shlomi-noach commented Mar 25, 2024

Uh oh!

codecov bot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dbussink left a comment

Choose a reason for hiding this comment

Uh oh!

shlomi-noach commented Mar 25, 2024

Uh oh!

GuptaManan100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshit-gangal Mar 26, 2024 • 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!

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!

shlomi-noach Mar 26, 2024 • 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!

shlomi-noach commented Mar 26, 2024

Uh oh!

shlomi-noach commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shlomi-noach commented Mar 25, 2024 •

edited

Loading

codecov bot commented Mar 25, 2024 •

edited

Loading

harshit-gangal Mar 26, 2024 •

edited

Loading

shlomi-noach Mar 26, 2024 •

edited

Loading

shlomi-noach commented Mar 26, 2024 •

edited

Loading