Add priority support to transaction throttler #12662

ejortegau · 2023-03-20T09:06:24Z

Description

This PR adds priority information support to the tablet's transaction throttler. It does so by:

Adding a query directive from which priority information is extracted.
Addin s vttablet flag to specify the default priority, to be used when queries lack (valid) priority query directive - defaulting to 100.

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

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

N/A

…pect that Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

harshit-gangal · 2023-03-28T04:26:22Z

proto/query.proto

+
+  // 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;


this should be an integer right?

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.

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

vitess/go/vt/vtctl/vtctl.go

Line 3495 in 39f83b9

customQuerySet := subFlags.Changed("custom-query")

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.

@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.

@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".

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.

@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?

that is the default behaviour today with throttler enabled.

Ah, right. That actually made sense, I was confused earlier.

go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go

harshit-gangal · 2023-03-28T04:38:16Z

go/vt/vttablet/tabletserver/tabletenv/config.go

+	fs.IntVar(&currentConfig.TxThrottlerDefaultCriticality, "tx-throttler-default-criticality", defaultConfig.TxThrottlerDefaultCriticality, "Default criticality assigned to queries that lack criticality information.")



do you see this change from the default of 0? Just seeing if we can avoid flag addition.

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.

go/vt/vttablet/tabletserver/tabletserver.go

harshit-gangal · 2023-03-28T04:41:12Z

go/vt/vttablet/tabletserver/tabletserver.go

@@ -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 != "" {


can call the proto method which checks for nil.

I am unsure what you mean here, can you please clarify?

go/vt/vttablet/tabletserver/txthrottler/tx_throttler_test.go

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

…ssio#12662 (#72)

harshit-gangal · 2023-04-19T18:42:53Z

You would need to update website docs about the new comment directive

ejortegau · 2023-04-20T07:35:13Z

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 <5791035+ejortegau@users.noreply.github.com>

derekperkins · 2023-04-28T12:38:43Z

Priority information is codified as a number between 0 and 100, where 0 corresponds to the lowest priority level and 100 to the highest

Not a huge deal, but Vitess messaging already has a Priority field, where 1 is the highest priority. Holistically, it would be nice if Vitess maintained a unified view of priority, and if this PR inverted the current definition to match, it would just mean the math changes slightly.

ejortegau · 2023-05-01T07:59:53Z

Priority information is codified as a number between 0 and 100, where 0 corresponds to the lowest priority level and 100 to the highest

Not a huge deal, but Vitess messaging already has a Priority field, where 1 is the highest priority. Holistically, it would be nice if Vitess maintained a unified view of priority, and if this PR inverted the current definition to match, it would just mean the math changes slightly.

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.

deepthi · 2023-05-01T16:21:03Z

Priority information is codified as a number between 0 and 100, where 0 corresponds to the lowest priority level and 100 to the highest

Not a huge deal, but Vitess messaging already has a Priority field, where 1 is the highest priority. Holistically, it would be nice if Vitess maintained a unified view of priority, and if this PR inverted the current definition to match, it would just mean the math changes slightly.

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 criticality not priority. While I agree with @derekperkins that it would be nice to have a unified notion of priority across Vitess, given the clear documentation of this new field I don't think it's a big deal either.

derekperkins · 2023-05-01T17:25:50Z

For me it is a bit counter intuitive the fact that the highest priority correposnds to the lowest value passed for the priority

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.

harshit-gangal · 2023-05-02T08:19:55Z

For me it is a bit counter intuitive the fact that the highest priority correposnds to the lowest value passed for the priority

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 <5791035+ejortegau@users.noreply.github.com>

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

ejortegau · 2023-05-03T14:15:17Z

For me it is a bit counter intuitive the fact that the highest priority correposnds to the lowest value passed for the priority

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.

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 <5791035+ejortegau@users.noreply.github.com>

…tler (#1448) * Add documentation for query priority support by the transaction throttler. This accompanies vitessio/vitess#12662 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comment. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Empty commit to re-run CI Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Update docs to reflect polarity inversion for priority directive. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Update content/en/docs/17.0/user-guides/configuration-advanced/comment-directives.md Signed-off-by: Harshit Gangal <harshit@planetscale.com> * Update content/en/docs/17.0/reference/programs/vttablet.md Signed-off-by: Harshit Gangal <harshit@planetscale.com> * empty commit to kick CI Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Empty commit to re-trigger CI Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * empty commit to kick CI Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * empty commit to kick CI Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Harshit Gangal <harshit@planetscale.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Harshit Gangal <harshit@planetscale.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* Add support for criticality query directive, and have TxThrottler respect that Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Remove unused variable Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix CI pipeline Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Make linter happy & add extra test cases. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix circular import Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments: * Invalid criticality in query directive fails the query. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Add missing piece of code that got lost during merge conflict resolution Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix vtadmin.js Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix unit tests (I think) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix flag e2e test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

…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 <5791035+ejortegau@users.noreply.github.com> * Remove unused variable Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix CI pipeline Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Make linter happy & add extra test cases. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix circular import Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments: * Invalid criticality in query directive fails the query. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Add missing piece of code that got lost during merge conflict resolution Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix vtadmin.js Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix unit tests (I think) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix flag e2e test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Add flag to select tx throttler tablet type (vitessio#12174) * Add flag to select tx throttler tablet type Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * REPLICA and/or RDONLY only Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update flag help msg Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Lowercase types in help/doc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Help update Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * fix test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * No underscores in flag Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix merge Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * PR suggestion, consolidate config logic Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update go/vt/vttablet/tabletserver/tabletenv/config.go Co-authored-by: Andrew Mason <amason@hey.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use topoproto.TabletTypeListFlag to handle flag Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix unit test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update go/vt/vttablet/tabletserver/tabletenv/config.go Co-authored-by: Andrew Mason <amason@hey.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * pr suggestions Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go fmt Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Andrew Mason <amason@hey.com> * txthrottler: further code cleanup (vitessio#12902) * txthrottler: further code cleanup Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix bad merge resolution Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Fix e2e flag tests & TxThrottler unit test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * 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 <5791035+ejortegau@users.noreply.github.com> * Add note on new TxThrottler behavior in v17 changelog Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix PR number in changelog entry for TxThrottler behavior change. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * txthrottler: verify config at vttablet startup, consolidate funcs (vitessio#13115) * txthrottler: verify config at vttablet startup, consolidate funcs Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use explicit dest in prototext.Unmarshal Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use for loop for TestVerifyTxThrottlerConfig Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix go vet complaint Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add back synonym flag Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Address staticcheck linter error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * gofumpt Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: Andrew Mason <amason@hey.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ejortegau added 2 commits March 16, 2023 12:04

Add support for criticality query directive, and have TxThrottler res…

7695299

…pect that Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

Remove unused variable

04c768d

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

ejortegau changed the title ~~Ejortegau/tx throttler supports criticality~~ Add criticality support to the transaction throttler Mar 20, 2023

Fix CI pipeline

84097bb

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

ejortegau marked this pull request as ready for review March 21, 2023 16:23

ejortegau requested review from deepthi, harshit-gangal, mattlord, systay, shlomi-noach, rohit-nayak-ps, frouioui and GuptaManan100 as code owners March 21, 2023 16:23

timvaillancourt mentioned this pull request Mar 22, 2023

slack-vitess-r14.0.4-dsdefense branch: initial cherry-picks, part 1 slackhq/vitess#54

Merged

4 tasks

harshit-gangal added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving NeedsWebsiteDocsUpdate What it says labels Mar 28, 2023

harshit-gangal reviewed Mar 28, 2023

View reviewed changes

go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go Outdated Show resolved Hide resolved

harshit-gangal reviewed Mar 28, 2023

View reviewed changes

go/vt/vttablet/tabletserver/tabletserver.go Outdated Show resolved Hide resolved

harshit-gangal reviewed Mar 28, 2023

View reviewed changes

go/vt/vttablet/tabletserver/txthrottler/tx_throttler_test.go Outdated Show resolved Hide resolved

Address PR comments.

7e8ffb5

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

ejortegau removed request for deepthi, systay, mattlord, shlomi-noach, frouioui and GuptaManan100 March 28, 2023 13:05

ejortegau requested a review from harshit-gangal April 19, 2023 12:29

timvaillancourt added a commit to slackhq/vitess that referenced this pull request Apr 19, 2023

slack-vitess-r14.0.5-dsdefense: backport directive rename from vite…

cfdbd1e

…ssio#12662 (#72)

harshit-gangal approved these changes Apr 19, 2023

View reviewed changes

ejortegau mentioned this pull request Apr 20, 2023

Add documentation for query priority support by the transaction throttler vitessio/website#1448

Merged

harshit-gangal removed the NeedsWebsiteDocsUpdate What it says label Apr 22, 2023

ejortegau changed the title ~~Add criticality support to the transaction throttler~~ priority May 1, 2023

ejortegau changed the title ~~priority~~ Add priority support to transaction throttler May 1, 2023

ejortegau added 2 commits May 3, 2023 16:00

Invert polarity of priority values

bdc89f7

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 <5791035+ejortegau@users.noreply.github.com>

Merge branch 'main' into ejortegau/tx_throttler_supports_criticality

0c94755

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

ejortegau requested a review from harshit-gangal May 3, 2023 14:15

ejortegau added 2 commits May 3, 2023 16:52

Make linter happy

a7e3882

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

Fix flag e2e test

4ecdd3b

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

harshit-gangal approved these changes May 4, 2023

View reviewed changes

harshit-gangal merged commit 3106f92 into vitessio:main May 4, 2023

timvaillancourt mentioned this pull request Apr 16, 2024

slack-vitess-r15.0.5: backport required Transaction Throttler PRs slackhq/vitess#302

Closed

4 tasks

timvaillancourt mentioned this pull request May 16, 2024

slack-vitess-r15.0.5: backport required Transaction Throttler PRs, pt. 2 slackhq/vitess#350

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add priority support to transaction throttler #12662

Add priority support to transaction throttler #12662

ejortegau commented Mar 20, 2023 •

edited

Loading

harshit-gangal Mar 28, 2023

ejortegau Mar 28, 2023 •

edited

Loading

shlomi-noach Mar 29, 2023

ejortegau Mar 29, 2023 •

edited

Loading

shlomi-noach Apr 3, 2023

shlomi-noach Apr 3, 2023

ejortegau Apr 3, 2023

shlomi-noach Apr 4, 2023 •

edited

Loading

harshit-gangal Apr 4, 2023

shlomi-noach Apr 4, 2023

harshit-gangal Mar 28, 2023

ejortegau Mar 28, 2023 •

edited

Loading

harshit-gangal Mar 28, 2023

ejortegau Mar 28, 2023

harshit-gangal commented Apr 19, 2023

ejortegau commented Apr 20, 2023

derekperkins commented Apr 28, 2023

ejortegau commented May 1, 2023

deepthi commented May 1, 2023

derekperkins commented May 1, 2023 •

edited

Loading

harshit-gangal commented May 2, 2023

ejortegau commented May 3, 2023

		fs.IntVar(&currentConfig.TxThrottlerDefaultCriticality, "tx-throttler-default-criticality", defaultConfig.TxThrottlerDefaultCriticality, "Default criticality assigned to queries that lack criticality information.")

Add priority support to transaction throttler #12662

Add priority support to transaction throttler #12662

Conversation

ejortegau commented Mar 20, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

Choose a reason for hiding this comment

ejortegau Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejortegau Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejortegau Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal commented Apr 19, 2023

ejortegau commented Apr 20, 2023

derekperkins commented Apr 28, 2023

ejortegau commented May 1, 2023

deepthi commented May 1, 2023

derekperkins commented May 1, 2023 • edited Loading

harshit-gangal commented May 2, 2023

ejortegau commented May 3, 2023

ejortegau commented Mar 20, 2023 •

edited

Loading

ejortegau Mar 28, 2023 •

edited

Loading

ejortegau Mar 29, 2023 •

edited

Loading

shlomi-noach Apr 4, 2023 •

edited

Loading

ejortegau Mar 28, 2023 •

edited

Loading

derekperkins commented May 1, 2023 •

edited

Loading