-
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
Tablet throttler: be explicit about client app name, exempt some apps from checks and heartbeat renewals #13195
Changes from 6 commits
78f98fc
379ec2b
1ef74c3
94b6f03
d242965
f978086
37a7c2f
3aab151
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,7 @@ import ( | |
"vitess.io/vitess/go/vt/vttablet/tabletserver/connpool" | ||
"vitess.io/vitess/go/vt/vttablet/tabletserver/tabletenv" | ||
"vitess.io/vitess/go/vt/vttablet/tabletserver/throttle" | ||
"vitess.io/vitess/go/vt/vttablet/tabletserver/throttle/throttlerapp" | ||
"vitess.io/vitess/go/vt/vttablet/tmclient" | ||
) | ||
|
||
|
@@ -124,7 +125,6 @@ var ( | |
migrationFailureFileName = "migration-failure.log" | ||
onlineDDLUser = "vt-online-ddl-internal" | ||
onlineDDLGrant = fmt.Sprintf("'%s'@'%s'", onlineDDLUser, "%") | ||
throttlerOnlineDDLApp = "online-ddl" | ||
throttleCheckFlags = &throttle.CheckFlags{} | ||
) | ||
|
||
|
@@ -1621,7 +1621,7 @@ exit $exit_code | |
fmt.Sprintf("--serve-socket-file=%s", serveSocketFile), | ||
fmt.Sprintf("--hooks-path=%s", tempDir), | ||
fmt.Sprintf(`--hooks-hint-token=%s`, onlineDDL.UUID), | ||
fmt.Sprintf(`--throttle-http=http://localhost:%d/throttler/check?app=%s:gh-ost:%s&p=low`, servenv.Port(), throttlerOnlineDDLApp, onlineDDL.UUID), | ||
fmt.Sprintf(`--throttle-http=http://localhost:%d/throttler/check?app=%s:%s:%s&p=low`, servenv.Port(), throttlerapp.OnlineDDLName, throttlerapp.GhostName, onlineDDL.UUID), | ||
fmt.Sprintf(`--database=%s`, e.dbName), | ||
fmt.Sprintf(`--table=%s`, onlineDDL.Table), | ||
fmt.Sprintf(`--alter=%s`, alterOptions), | ||
|
@@ -1768,7 +1768,7 @@ export MYSQL_PWD | |
my ($self, %args) = @_; | ||
|
||
return sub { | ||
if (head("http://localhost:{{VTTABLET_PORT}}/throttler/check?app={{THROTTLER_ONLINE_DDL_APP}}:pt-osc:{{MIGRATION_UUID}}&p=low")) { | ||
if (head("http://localhost:{{VTTABLET_PORT}}/throttler/check?app={{THROTTLER_ONLINE_DDL_APP}}:{{THROTTLER_PT_OSC_APP}}:{{MIGRATION_UUID}}&p=low")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that we didn't support pt-osc? Might be a good opportunity to clean up the code if so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's labeled as "experimental". I'd rather not do everything in this PR. We can deprecate pt-osc later. |
||
# Got HTTP 200 OK, means throttler is happy | ||
return 0; | ||
} else { | ||
|
@@ -1782,7 +1782,8 @@ export MYSQL_PWD | |
` | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{VTTABLET_PORT}}", fmt.Sprintf("%d", servenv.Port())) | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{MIGRATION_UUID}}", onlineDDL.UUID) | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{THROTTLER_ONLINE_DDL_APP}}", throttlerOnlineDDLApp) | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{THROTTLER_ONLINE_DDL_APP}}", throttlerapp.OnlineDDLName) | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{THROTTLER_PT_OSC_APP}}", throttlerapp.PTOSCName) | ||
|
||
pluginCode = strings.ReplaceAll(pluginCode, "{{OnlineDDLStatusRunning}}", string(schema.OnlineDDLStatusRunning)) | ||
pluginCode = strings.ReplaceAll(pluginCode, "{{OnlineDDLStatusComplete}}", string(schema.OnlineDDLStatusComplete)) | ||
|
@@ -2141,7 +2142,7 @@ func (e *Executor) ThrottleAllMigrations(ctx context.Context, expireString strin | |
if err := e.lagThrottler.CheckIsReady(); err != nil { | ||
return nil, err | ||
} | ||
_ = e.lagThrottler.ThrottleApp(throttlerOnlineDDLApp, time.Now().Add(duration), ratio) | ||
_ = e.lagThrottler.ThrottleApp(throttlerapp.OnlineDDLName, time.Now().Add(duration), ratio) | ||
return emptyResult, nil | ||
} | ||
|
||
|
@@ -2161,7 +2162,7 @@ func (e *Executor) UnthrottleAllMigrations(ctx context.Context) (result *sqltype | |
return nil, err | ||
} | ||
defer e.triggerNextCheckInterval() | ||
_ = e.lagThrottler.UnthrottleApp(throttlerOnlineDDLApp) | ||
_ = e.lagThrottler.UnthrottleApp(throttlerapp.OnlineDDLName) | ||
return emptyResult, nil | ||
} | ||
|
||
|
@@ -3469,7 +3470,7 @@ func (e *Executor) reviewRunningMigrations(ctx context.Context) (countRunnning i | |
if err := e.lagThrottler.CheckIsReady(); err == nil { | ||
// No point in reviewing throttler info if it's not enabled&open | ||
for _, app := range e.lagThrottler.ThrottledApps() { | ||
if app.AppName == throttlerOnlineDDLApp { | ||
if app.AppName == throttlerapp.OnlineDDLName { | ||
currentUserThrottleRatio = app.Ratio | ||
break | ||
} | ||
|
@@ -3603,7 +3604,7 @@ func (e *Executor) reviewRunningMigrations(ctx context.Context) (countRunnning i | |
// - it's a deadlock. | ||
// And so, once per reviewRunningMigrations(), and assuming there _are_ running migrations, we ensure to hit a throttler check. This will kick | ||
// on-demand heartbeats, unlocking the deadlock. | ||
e.lagThrottler.CheckByType(ctx, throttlerOnlineDDLApp, "", throttleCheckFlags, throttle.ThrottleCheckPrimaryWrite) | ||
e.lagThrottler.CheckByType(ctx, throttlerapp.OnlineDDLName, "", throttleCheckFlags, throttle.ThrottleCheckPrimaryWrite) | ||
}) | ||
} | ||
} | ||
|
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.
IMO it might be worth making a new type that's an alias for string so that what the string represents is even more explicit. This make it clear to future code writers that we're not using arbitrary strings.
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.
You're right. I initially did that but then that affected so many lines of code, casting into and out of string, that I decided to hold off. I can still do that for this PR if you think it's worhtwhile.
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'll leave it up to you. I think it's probably worth it, but we can always do it later too.
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.
Done in 37a7c2f. I'm not sure how I feel about it, what do you think?