-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block replication and query RPC calls until wait for dba grants has completed #14836
Block replication and query RPC calls until wait for dba grants has completed #14836
Conversation
… has succeeded Signed-off-by: Manan Gupta <manan@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
…errors Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this solution. Would it make sense to handle/wait for this initialization work here?
vitess/go/vt/vttablet/tabletserver/connpool/pool.go
Lines 67 to 96 in 04028a9
// NewPool creates a new Pool. The name is used | |
// to publish stats only. | |
func NewPool(env tabletenv.Env, name string, cfg tabletenv.ConnPoolConfig) *Pool { | |
cp := &Pool{ | |
timeout: cfg.Timeout, | |
env: env, | |
} | |
config := smartconnpool.Config[*Conn]{ | |
Capacity: int64(cfg.Size), | |
IdleTimeout: cfg.IdleTimeout, | |
MaxLifetime: cfg.MaxLifetime, | |
RefreshInterval: mysqlctl.PoolDynamicHostnameResolution, | |
} | |
if name != "" { | |
config.LogWait = func(start time.Time) { | |
env.Stats().WaitTimings.Record(name+"ResourceWaitTime", start) | |
} | |
cp.getConnTime = env.Exporter().NewTimings(name+"GetConnTime", "Tracks the amount of time it takes to get a connection", "Settings") | |
} | |
cp.ConnPool = smartconnpool.NewPool(&config) | |
cp.ConnPool.RegisterStats(env.Exporter(), name) | |
cp.dbaPool = dbconnpool.NewConnectionPool("", env.Exporter(), 1, config.IdleTimeout, config.MaxLifetime, 0) | |
return cp | |
} |
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
The thing with |
Signed-off-by: Manan Gupta <manan@planetscale.com>
5c3af79
to
48bddab
Compare
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I think the alternatives considered are worse than this solution. Guess the least terrible solution for now.
Description
This PR tries to fix the issue pointed out in #14834.
This is an extension of #14565 and #14680.
The proposed fix in this PR is to block all the replication and query based RPC calls from running until the
waitForDBAGrants
has succeeded. To this end, we have introduced a new member in the tabletmanager struct which is a channel that we use to know if the function call has finished or not.Since
waitForDBAGrants
was a function only being used in the tabletmanager package, we have moved it there and made it a local function. This is also good because it now needs access to the newly added channel variable.This change ensures that we have verified and waited for the users to have the correct grants when we start running replication and query RPC calls.
This change has the added benefit of not re-introducing the problem in #14681
Related Issue(s)
Checklist
Deployment Notes