[vtadmin] Removing explicit Dial-ing for vtsql proxy#10237
Merged
ajm188 merged 8 commits intovitessio:mainfrom May 9, 2022
Merged
[vtadmin] Removing explicit Dial-ing for vtsql proxy#10237ajm188 merged 8 commits intovitessio:mainfrom
ajm188 merged 8 commits intovitessio:mainfrom
Conversation
ajm188
added
Type: Internal Cleanup
Component: VTAdmin
added 8 commits
May 9, 2022 14:04
Signed-off-by: Andrew Mason <andrew@planetscale.com>
…ial` Signed-off-by: Andrew Mason <andrew@planetscale.com>
This was only testing that Dial-ing during GetTablets could fail, but we're removing that call, so this test doesn't actually check anything anymore. Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
ajm188
force-pushed
the
andrew/vtadmin/vtsql-nodial
branch
from
41ab024 to
abdbbea
Compare
ajm188
requested review from
doeg,
frouioui,
harshit-gangal,
notfelineit,
rohit-nayak-ps and
systay
as code owners
doeg
approved these changes
May 9, 2022
| func DialProtocol(ctx context.Context, protocol string, address string) (*VTGateConn, error) { | ||
| dialersM.Lock() | ||
| dialer, ok := dialers[protocol] | ||
| dialersM.Unlock() |
Contributor
There was a problem hiding this comment.
Is there a reason for not using defer beyond "it's like, one line"? (Purely curious/not a leading question lol.)
Contributor
Author
There was a problem hiding this comment.
because we don't need to hold the mutex to invoke the dialer, just to get it out of the map
Contributor
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This builds on #10233, but for vtsql this time (it's based on the same branch, so it should be merged after that PR).
It's the same changes, applied in the same order (config options, context plumbing through the call stack, etc etc) but applied to the
DBinterface andVTGateProxytype instead. In addition:vtgateconn's privatedriversmap thread safe. this was tripping the race detector during tests (see 69cf2f2975585025aea1cc7ab58d35aa1d5b4dfa)Related Issue(s)
Checklist
Deployment Notes