-
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
TxThrottler support for transactions outside BEGIN/COMMIT #13040
TxThrottler support for transactions outside BEGIN/COMMIT #13040
Conversation
This change allows the TxThrottler to throttle requests sent outside of explicit transactions (i.e. explicit BEGIN/COMMIT blocks) when configured to do so via a new config flag. Otherwise, it preserves the current/default behavior of only throttling transactions inside BEGIN/COMMIT. In addition, when this flag is passed, and because the call to throttle is done in a context in which the execution plan is already known, this change uses the plan type to make sure that throttling is triggered only when the query being executed is INSERT/UPDATE/DELETE/LOAD, so that SELECTs and others no longer get throttled unnecessarily, as they do not contribute to increasing replication lag, which is what the TxThrottler aims at controlling. Signed-off-by: Eduardo J. Ortega U <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
Signed-off-by: Eduardo J. Ortega U <[email protected]>
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.
Let's decide how to proceed with the feature based on the comment here #13037
This changes where we call the transaction throttler: 1. Statements in `BEGIN/COMMIT` blocks keep being throttled in `TabletServer.begin()`. 2. Additionally, throttling is added in QueryExecutor.execAutocommit() and `QueryExecutor.execAsTransaction()`. We also change things so that throttling in this new case is not opt-in via configuration flag but instead is the new and only behavior. Finally, we remove some previously added changes to example scripts that had been added with the intention of testing and are not part of the PR. Signed-off-by: Eduardo J. Ortega U <[email protected]>
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.
changes look good. It would need a few tests to validate the expectations.
Also, a good idea to update the release note in |
To make unit testing simple here, we separated the interface and implementation of the TxThrottle, and simply used a mock implementation of the interface in the tests. Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
static check is failing
|
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
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.
LGTM with a minor styling request
@@ -539,6 +525,24 @@ func (tsv *TabletServer) begin(ctx context.Context, target *querypb.Target, save | |||
return state, err | |||
} | |||
|
|||
func (tsv *TabletServer) getPriorityFromOptions(options *querypb.ExecuteOptions) int { | |||
priority := tsv.config.TxThrottlerDefaultPriority | |||
if options != nil && options.Priority != "" { |
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.
A small nit: in vitess
codebase we prefer early returns over indented conditions, so for example we'd do this:
if options != nil && options.Priority != "" { | |
if options == nil { | |
return priority | |
} | |
if options.Priority == "" { | |
return priority | |
} |
Mind that if you accept the above suggestion, the code will not compile as we need t remove a closing brace }
Signed-off-by: Eduardo J. Ortega U <[email protected]>
…3040) * TxThrottler support for transactions outside BEGIN/COMMIT This change allows the TxThrottler to throttle requests sent outside of explicit transactions (i.e. explicit BEGIN/COMMIT blocks) when configured to do so via a new config flag. Otherwise, it preserves the current/default behavior of only throttling transactions inside BEGIN/COMMIT. In addition, when this flag is passed, and because the call to throttle is done in a context in which the execution plan is already known, this change uses the plan type to make sure that throttling is triggered only when the query being executed is INSERT/UPDATE/DELETE/LOAD, so that SELECTs and others no longer get throttled unnecessarily, as they do not contribute to increasing replication lag, which is what the TxThrottler aims at controlling. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix e2e flag tests & TxThrottler unit test Signed-off-by: Eduardo J. Ortega U <[email protected]> * Throttle auto-commit statements in QueryExecutor instead of TxPool This changes where we call the transaction throttler: 1. Statements in `BEGIN/COMMIT` blocks keep being throttled in `TabletServer.begin()`. 2. Additionally, throttling is added in QueryExecutor.execAutocommit() and `QueryExecutor.execAsTransaction()`. We also change things so that throttling in this new case is not opt-in via configuration flag but instead is the new and only behavior. Finally, we remove some previously added changes to example scripts that had been added with the intention of testing and are not part of the PR. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Adds test cases for QueryExecutor.Execute() with TxThrottle throttling To make unit testing simple here, we separated the interface and implementation of the TxThrottle, and simply used a mock implementation of the interface in the tests. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add note on new TxThrottler behavior in v17 changelog Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix PR number in changelog entry for TxThrottler behavior change. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]>
…3040) * TxThrottler support for transactions outside BEGIN/COMMIT This change allows the TxThrottler to throttle requests sent outside of explicit transactions (i.e. explicit BEGIN/COMMIT blocks) when configured to do so via a new config flag. Otherwise, it preserves the current/default behavior of only throttling transactions inside BEGIN/COMMIT. In addition, when this flag is passed, and because the call to throttle is done in a context in which the execution plan is already known, this change uses the plan type to make sure that throttling is triggered only when the query being executed is INSERT/UPDATE/DELETE/LOAD, so that SELECTs and others no longer get throttled unnecessarily, as they do not contribute to increasing replication lag, which is what the TxThrottler aims at controlling. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix e2e flag tests & TxThrottler unit test Signed-off-by: Eduardo J. Ortega U <[email protected]> * Throttle auto-commit statements in QueryExecutor instead of TxPool This changes where we call the transaction throttler: 1. Statements in `BEGIN/COMMIT` blocks keep being throttled in `TabletServer.begin()`. 2. Additionally, throttling is added in QueryExecutor.execAutocommit() and `QueryExecutor.execAsTransaction()`. We also change things so that throttling in this new case is not opt-in via configuration flag but instead is the new and only behavior. Finally, we remove some previously added changes to example scripts that had been added with the intention of testing and are not part of the PR. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Adds test cases for QueryExecutor.Execute() with TxThrottle throttling To make unit testing simple here, we separated the interface and implementation of the TxThrottle, and simply used a mock implementation of the interface in the tests. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add note on new TxThrottler behavior in v17 changelog Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix PR number in changelog entry for TxThrottler behavior change. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]>
…3040) * TxThrottler support for transactions outside BEGIN/COMMIT This change allows the TxThrottler to throttle requests sent outside of explicit transactions (i.e. explicit BEGIN/COMMIT blocks) when configured to do so via a new config flag. Otherwise, it preserves the current/default behavior of only throttling transactions inside BEGIN/COMMIT. In addition, when this flag is passed, and because the call to throttle is done in a context in which the execution plan is already known, this change uses the plan type to make sure that throttling is triggered only when the query being executed is INSERT/UPDATE/DELETE/LOAD, so that SELECTs and others no longer get throttled unnecessarily, as they do not contribute to increasing replication lag, which is what the TxThrottler aims at controlling. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix e2e flag tests & TxThrottler unit test Signed-off-by: Eduardo J. Ortega U <[email protected]> * Throttle auto-commit statements in QueryExecutor instead of TxPool This changes where we call the transaction throttler: 1. Statements in `BEGIN/COMMIT` blocks keep being throttled in `TabletServer.begin()`. 2. Additionally, throttling is added in QueryExecutor.execAutocommit() and `QueryExecutor.execAsTransaction()`. We also change things so that throttling in this new case is not opt-in via configuration flag but instead is the new and only behavior. Finally, we remove some previously added changes to example scripts that had been added with the intention of testing and are not part of the PR. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Adds test cases for QueryExecutor.Execute() with TxThrottle throttling To make unit testing simple here, we separated the interface and implementation of the TxThrottle, and simply used a mock implementation of the interface in the tests. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add note on new TxThrottler behavior in v17 changelog Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix PR number in changelog entry for TxThrottler behavior change. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]>
…pt. 2 (#350) * Add priority support to transaction throttler (vitessio#12662) * Add support for criticality query directive, and have TxThrottler respect that Signed-off-by: Eduardo J. Ortega U <[email protected]> * Remove unused variable Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix CI pipeline Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy & add extra test cases. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix circular import Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments: * Invalid criticality in query directive fails the query. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments: * Renamed criticality to priority. * Change error handling when parsing the priority from string to integer. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add missing piece of code that got lost during merge conflict resolution Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix vtadmin.js Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix unit tests (I think) Signed-off-by: Eduardo J. Ortega U <[email protected]> * Invert polarity of priority values With this change, queries with PRIORITY=0 never get throttled, whereas those with PRIORITY=100 always do (provided there is contention). Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix flag e2e test Signed-off-by: Eduardo J. Ortega U <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add flag to select tx throttler tablet type (vitessio#12174) * Add flag to select tx throttler tablet type Signed-off-by: Tim Vaillancourt <[email protected]> * REPLICA and/or RDONLY only Signed-off-by: Tim Vaillancourt <[email protected]> * Update flag help msg Signed-off-by: Tim Vaillancourt <[email protected]> * Lowercase types in help/doc Signed-off-by: Tim Vaillancourt <[email protected]> * Help update Signed-off-by: Tim Vaillancourt <[email protected]> * fix test Signed-off-by: Tim Vaillancourt <[email protected]> * No underscores in flag Signed-off-by: Tim Vaillancourt <[email protected]> * Fix test Signed-off-by: Tim Vaillancourt <[email protected]> * Fix merge Signed-off-by: Tim Vaillancourt <[email protected]> * PR suggestion, consolidate config logic Signed-off-by: Tim Vaillancourt <[email protected]> * Update go/vt/vttablet/tabletserver/tabletenv/config.go Co-authored-by: Andrew Mason <[email protected]> Signed-off-by: Tim Vaillancourt <[email protected]> * Use topoproto.TabletTypeListFlag to handle flag Signed-off-by: Tim Vaillancourt <[email protected]> * Fix unit test Signed-off-by: Tim Vaillancourt <[email protected]> * Update go/vt/vttablet/tabletserver/tabletenv/config.go Co-authored-by: Andrew Mason <[email protected]> Signed-off-by: Tim Vaillancourt <[email protected]> * improve test Signed-off-by: Tim Vaillancourt <[email protected]> * pr suggestions Signed-off-by: Tim Vaillancourt <[email protected]> * go fmt Signed-off-by: Tim Vaillancourt <[email protected]> --------- Signed-off-by: Tim Vaillancourt <[email protected]> Co-authored-by: Andrew Mason <[email protected]> * txthrottler: further code cleanup (vitessio#12902) * txthrottler: further code cleanup Signed-off-by: Tim Vaillancourt <[email protected]> * Fix bad merge resolution Signed-off-by: Tim Vaillancourt <[email protected]> --------- Signed-off-by: Tim Vaillancourt <[email protected]> * TxThrottler support for transactions outside BEGIN/COMMIT (vitessio#13040) * TxThrottler support for transactions outside BEGIN/COMMIT This change allows the TxThrottler to throttle requests sent outside of explicit transactions (i.e. explicit BEGIN/COMMIT blocks) when configured to do so via a new config flag. Otherwise, it preserves the current/default behavior of only throttling transactions inside BEGIN/COMMIT. In addition, when this flag is passed, and because the call to throttle is done in a context in which the execution plan is already known, this change uses the plan type to make sure that throttling is triggered only when the query being executed is INSERT/UPDATE/DELETE/LOAD, so that SELECTs and others no longer get throttled unnecessarily, as they do not contribute to increasing replication lag, which is what the TxThrottler aims at controlling. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix e2e flag tests & TxThrottler unit test Signed-off-by: Eduardo J. Ortega U <[email protected]> * Throttle auto-commit statements in QueryExecutor instead of TxPool This changes where we call the transaction throttler: 1. Statements in `BEGIN/COMMIT` blocks keep being throttled in `TabletServer.begin()`. 2. Additionally, throttling is added in QueryExecutor.execAutocommit() and `QueryExecutor.execAsTransaction()`. We also change things so that throttling in this new case is not opt-in via configuration flag but instead is the new and only behavior. Finally, we remove some previously added changes to example scripts that had been added with the intention of testing and are not part of the PR. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Adds test cases for QueryExecutor.Execute() with TxThrottle throttling To make unit testing simple here, we separated the interface and implementation of the TxThrottle, and simply used a mock implementation of the interface in the tests. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Add note on new TxThrottler behavior in v17 changelog Signed-off-by: Eduardo J. Ortega U <[email protected]> * Fix PR number in changelog entry for TxThrottler behavior change. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Make linter happy Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]> * txthrottler: verify config at vttablet startup, consolidate funcs (vitessio#13115) * txthrottler: verify config at vttablet startup, consolidate funcs Signed-off-by: Tim Vaillancourt <[email protected]> * Use explicit dest in prototext.Unmarshal Signed-off-by: Tim Vaillancourt <[email protected]> * Use for loop for TestVerifyTxThrottlerConfig Signed-off-by: Tim Vaillancourt <[email protected]> * Cleanup test Signed-off-by: Tim Vaillancourt <[email protected]> * Fix go vet complaint Signed-off-by: Tim Vaillancourt <[email protected]> * Add back synonym flag Signed-off-by: Tim Vaillancourt <[email protected]> * Update go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go Co-authored-by: Shlomi Noach <[email protected]> Signed-off-by: Tim Vaillancourt <[email protected]> * Address staticcheck linter error Signed-off-by: Tim Vaillancourt <[email protected]> --------- Signed-off-by: Tim Vaillancourt <[email protected]> Co-authored-by: Shlomi Noach <[email protected]> * gofumpt Signed-off-by: Tim Vaillancourt <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]> Signed-off-by: Tim Vaillancourt <[email protected]> Co-authored-by: Eduardo J. Ortega U <[email protected]> Co-authored-by: Andrew Mason <[email protected]> Co-authored-by: Shlomi Noach <[email protected]>
Description
This change allows the
TxThrottler
to throttle requests sent outside of explicit transactions (i.e. explicitBEGIN/COMMIT
blocks).Related Issue(s)
#13037
Checklist
Deployment Notes
Any existing deployments that use the transaction throttler (
--enable-tx-throttler
) will see it now throttleINSERT/UPDATE/DELETE
instructions even if they are not inside of aBEGIN/COMMIT
block. This changes the behavior with respect to previous versions.