-
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
Add priority support to transaction throttler #12662
Add priority support to transaction throttler #12662
Conversation
…pect that 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]>
proto/query.proto
Outdated
|
||
// criticality specifies the criticality of the query, between 0 and 100. This is leveraged by the transaction | ||
// throttler to determine whether, under resource contention, a query should or should not be throttled. | ||
string criticality = 16; |
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.
this should be an integer right?
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.
No; IMHO, if it is not passed by the client in the query comments, the integer will end up being 0
in vttablet
as if the client had specified criticality was 0
instead of not being specified - in which case it should use whatever is set in the default criticality flag.
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.
the integer will end up being 0 in vttablet
Do you mean as a command line flag? There is a mechanism to identify whether an integer command line flag was entered as 0
or not entered at all. see
Line 3495 in 39f83b9
customQuerySet := subFlags.Changed("custom-query") |
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 am not sure that's enough/would work, but maybe I am misunderstanding something. Let's say that criticality is defined as integer. This is parsed in the vtgate from the query directives and there are two options of interest for what I see as a potential issue:
- It is not present.
- It is passed by the client as zero.
If it is not present, it is not sent to the vttablet. Eventually the message becomes a go struct and because the data is not in the underlying proto message, the attribute of the query.ExecuteOptions
struct is set to zero.
If criticality is passed by client in the comments as zero, it is set to zero in the message, sent to vttablet, and set to zero in the query.ExecuteOptions
struct.
With this approach, vttablet
has no way to determine whether the criticality is zero because it was not passed in the query or because that's what the user set on the comments. In the first case, it should overwrite it with what is set in the corresponding flag (which exists on the vttablet, but not in vtgate). On the second one, it should leave it as it is.
When passed as a string, the empty string ""
is clearly distinguishable from the string with a zero "0"
, so vttablet knows whether to use the default criticality or use the zero in the ExecuteOptions
struct.
An alternative is to set the flag for default criticality a part of vtgate instead of vttablet and always send them in the execute options, but from a previous PR, I was under the impression that we didn't want to send values that were not explicitly set.
Another option is to have vtgate pass a "special" integer value (e.g. -1) if criticality is not present in the query directives and replace that in the vttablet with the default criticality. I can do that if you prefer, though to me it seems functionally equivalent to passing the empty string and checking that to potentially override the value in the vttablet.
Then again, maybe I am missing something here.
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.
@ejortegau you're right that this is more complex than I thought. My previous experience with similar problems was that vtgate
or vtctl
did have access to the existing config (e.g. via topo
) and could therefore send a complete and correct config object to vttablet
.
This, an the fact that 0
is a perfectly valid value (where other times, it's a clear indicator for "no value").
I completely understand the logic of using an empty string as an indicator for "no value". I think a negative integer value is a good option. The thing with a string is the risk for errors, and the conventional idiom of always checking for errors, then reporting them, etc., which complicates the code.
I personally prefer a negative integer value as an indicator. If it's a sensible and trivial change for you, great, and let's do that. If it only complicates matters, forget it; it's not a hill to die on.
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.
@harshit-gangal in my understanding you will have higher priority operations (like your normal OLTP) and lower level operations, where you may choose a different priority. And in my understanding 0
, the default, is "highest 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.
With the current implementation, 0 is the lowest.
The reason why we want to have the default to something different than 0 is onboarding. When onboarding, many/most/all of your queries will lack criticality information, and you don't want them to start being throttled. Hence the default we will is when onboarding will be 100 (highest, corresponding to never throttle). As we make progress, our client app queries will start sending their own, actual criticalities. Eventually, we will no longer need to fill in the missing criticality from the query with a high default one, and instead can move it to be the lowest (the default of 0). This is the use case we have for it.
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.
@ejortegau I'm just trying to think about how this will behave for other Vitess users who are unaware of your specific intentions. For those users, if they ever activate tx throttler, their queries will have criticality 0
, meaning "always throttled" -- did I understand correctly?
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.
that is the default behaviour today with throttler enabled.
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.
Ah, right. That actually made sense, I was confused earlier.
fs.IntVar(¤tConfig.TxThrottlerDefaultCriticality, "tx-throttler-default-criticality", defaultConfig.TxThrottlerDefaultCriticality, "Default criticality assigned to queries that lack criticality information.") | ||
|
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.
do you see this change from the default of 0
? Just seeing if we can avoid flag addition.
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.
Yeah, we will start with 100 and then have our apps start sending different values. It facilitates onboarding without breaking anything, as nothing gets throttled if the default is 100.
@@ -490,7 +490,14 @@ func (tsv *TabletServer) begin(ctx context.Context, target *querypb.Target, save | |||
target, options, false, /* allowOnShutdown */ | |||
func(ctx context.Context, logStats *tabletenv.LogStats) error { | |||
startTime := time.Now() | |||
if tsv.txThrottler.Throttle() { | |||
criticality := tsv.config.TxThrottlerDefaultCriticality | |||
if options != nil && options.Criticality != "" { |
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.
can call the proto method which checks for nil.
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 am unsure what you mean here, can you please clarify?
Signed-off-by: Eduardo J. Ortega U <[email protected]>
You would need to update website docs about the new comment directive |
Sure, I will file a separate PR for it today |
…tler. This accompanies vitessio/vitess#12662 Signed-off-by: Eduardo J. Ortega U <[email protected]>
Not a huge deal, but Vitess messaging already has a |
For me it is a bit counter intuitive the fact that the highest priority correposnds to the lowest value passed for the priority. I realize that there are different contexts in which that's how it's used, as in the example you gave; but unless folks at large feel strongly about this one, I'd prefer to stick to high priority value corresponding to high priority, and low priority value corresponding to low priority. |
Some of this is stemming from the fact that this was originally conceived as |
It can definitely be ambiguous, but any reference to a number 1 priority always means the highest priority, hence the decision in messaging. Again, I don't feel super strongly about it and am happy to defer to @deepthi for a decision, I just wanted it to be an informed decision. |
I agree with @derekperkins here. Priority is usually: lower the number higher the priority. The 100th priority looks like the least priority in writing. This is the reason the Priority number should be mapped to textual form as it takes away this ambiguity. |
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]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
I have inverted the polarity on handling priority values to make 0 the highest priority and 100 the lowest one. |
Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Eduardo J. Ortega U <[email protected]>
…tler (#1448) * Add documentation for query priority support by the transaction throttler. This accompanies vitessio/vitess#12662 Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comments Signed-off-by: Eduardo J. Ortega U <[email protected]> * Address PR comment. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Empty commit to re-run CI Signed-off-by: Eduardo J. Ortega U <[email protected]> * Update docs to reflect polarity inversion for priority directive. Signed-off-by: Eduardo J. Ortega U <[email protected]> * Update content/en/docs/17.0/user-guides/configuration-advanced/comment-directives.md Signed-off-by: Harshit Gangal <[email protected]> * Update content/en/docs/17.0/reference/programs/vttablet.md Signed-off-by: Harshit Gangal <[email protected]> * empty commit to kick CI Signed-off-by: Shlomi Noach <[email protected]> * Empty commit to re-trigger CI Signed-off-by: Eduardo J. Ortega U <[email protected]> * empty commit to kick CI Signed-off-by: Shlomi Noach <[email protected]> * empty commit to kick CI Signed-off-by: Shlomi Noach <[email protected]> --------- Signed-off-by: Eduardo J. Ortega U <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Shlomi Noach <[email protected]> Co-authored-by: Harshit Gangal <[email protected]> Co-authored-by: Shlomi Noach <[email protected]>
* 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 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]>
…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 PR adds priority information support to the tablet's transaction throttler. It does so by:
vttablet
flag to specify the default priority, to be used when queries lack (valid) priority query directive - defaulting to100
.The implementation is backwards compatible in the sense that if no query directives are passed and no default priority information is defined for the
vttablet
, it preserves the current behavior of the transaction throttler.Priority information is codified as a number between 0 and 100, where 0 corresponds to the highest priority level and 100 to the lowest. If the current transaction throttler would throttle a query, now it considers the priority to decide whether to actually throttle it or not. This is done by using the query's priority as the probability to throttle and using that to decide whether to throttle.
Related Issue(s)
#12661
Checklist
Deployment Notes
N/A