Fix transaction throttler ignoring the initial rate by ejortegau · Pull Request #12618 · vitessio/vitess

ejortegau · 2023-03-13T16:52:40Z

Description

This PR fixes an issue leading to the transaction throttler ignoring the initial rate parameter passed to it.

Related Issue(s)

#12549

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

This addresses the issue reported in vitessio#12549 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

vitess-bot · 2023-03-13T16:52:43Z

…r()` 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>

…rate 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>

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

shlomi-noach · 2023-03-16T15:40:28Z

go/vt/throttler/throttler.go

-		return nil, fmt.Errorf("maxReplicationLag must be >= 0: %v", maxReplicationLag)
+	if maxRateModuleMaxRate < 0 {
+		return nil, fmt.Errorf("maxRate must be >= 0: %v", maxRateModuleMaxRate)
 	}


Was the maxReplicationLag sanity check removed intentionally? I believe we should keep validating that the value is non-negative?

Yes. this was removed intentionally. Instead, we call MaxReplicationLagModuleConfig.Verify() here, which already imposes stronger constraints on both target and max replication lag.

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

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

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

shlomi-noach

LGTM

* Fix transaction throttler ignoring the initial rate This addresses the issue reported in vitessio#12549 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Add missing override of max replication lag in `throttler.newThrottler()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Reorder functions to make diff easier to read Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix check for maxRate in `newThrottlerFromConfig()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix some CI pipeline issues 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> * Fix typo 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> Signed-off-by: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com>

* Add basic metrics to `vttablet` transaction throttler (vitessio#12418) * Add basic stats to vttablet tx throttler Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * test new metrics Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * reorder Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * short names Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add max rate Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Move NewGaugeFunc to under conditional Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use env Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Remove env from TxThrottler struct Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix tests Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * PR suggestion Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix unit test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * reorder test vars Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix transaction throttler ignoring the initial rate (vitessio#12618) * Fix transaction throttler ignoring the initial rate This addresses the issue reported in vitessio#12549 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Add missing override of max replication lag in `throttler.newThrottler()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Reorder functions to make diff easier to read Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix check for maxRate in `newThrottlerFromConfig()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix some CI pipeline issues 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> * Fix typo 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> Signed-off-by: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com> * Cleanup panics in `txthrottler`, reorder for readability (vitessio#12901) * Cleanup tx_throttler.go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup tx_throttler.go #2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix throttlerFactoryFunc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo if-cond consolidation Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo struct shuffling Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * prove that disabled config returns nil error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Emit per workload labels for existing per table vttablet metrics (vitessio#12394) * Emit per workload labels for existing per table vttablet metrics This adds the possibility to configure vttablet (via CLI flag) to also have a workload label for existing per table metrics (query counts, query times, query errors, query rows affected, query rows returned, query error counts). Workload can be any string that makes sense for the client application. For example, API endpoint name, controller, batch job name, application name or something else. This is usefult to be able to gain observability about how the query load is distributed across different workloads. This is achieved with two new CLI flags, namely: * `enable-per-workload-table-metrics`: whether to enable or disable per workload metric collection - disabled by default to preserve the current behavior, thus making the new feature opt-in only. * `workload-label`: a string to look for in query comments to identify the workload running the current query. The workload is obtained by parsing query comments of the form: /* ... <workload_label>=<workload_name>; ... */ For example, if vttablet is started with `--enable-per-workload-table-metrics --workload-label app_name` anda query is issued with a comment like /* ... app_name=shop; ... */ then metrics will look like ``` vttablet_query_counts{plan="Select",table="dual", workload="shop"} 15479 ``` instead of ``` vttablet_query_counts{plan="Select",table="dual"} 15479 ``` Query comment parsing only takes place if `--enable-per-workload-table-metrics` is used, as to not incur parsing performance impact if the user does not want per workload metrics. 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 flags e2e test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments: * Obtain workload information on the vtgate instead of the vttablet, avoiding double parsing. * Treat workload name as a query directive. * Send workload name from vtgate to vttablet as ExecuteOptions. Additionally, annotate tabletserver's execution span with the workload name to also enrich traces with workload name data, in addition to metrics. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * A few fixes: 1. Rebuild some files with `make proto`. 2. Protect against nil ExecuteOptions on the tabletserver. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix flags e2e test 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> * Fixes Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix a comment Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix e2e flag test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Update JS code for protobuf changes. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix QueryEngine unit test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix e2e flag test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix spurious tab in comment Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comment Don't use dual format flag for new flags - stick with - separated ones. 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> * remove mistaken git add Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * make vtadmin_web_proto_types Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * test unit_race test on go-version: 1.18.9 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Revert "test unit_race test on go-version: 1.18.9" This reverts commit 922e897. * CI: Misc test improvements to limit failures with various runners (vitessio#13825) Signed-off-by: Matt Lord <mattalord@gmail.com> * Fix setup order to avoid races (vitessio#13871) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> 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> Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>

Fix transaction throttler ignoring the initial rate

1744f05

This addresses the issue reported in vitessio#12549 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 13, 2023

ejortegau marked this pull request as ready for review March 13, 2023 16:52

ejortegau requested review from deepthi, harshit-gangal, rohit-nayak-ps, shlomi-noach and systay as code owners March 13, 2023 16:52

ejortegau added 4 commits March 13, 2023 17:54

Add missing override of max replication lag in `throttler.newThrottle…

e2cb501

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

Reorder functions to make diff easier to read

47ed606

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

Fix check for maxRate in newThrottlerFromConfig()

3fb8f96

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

Merge branch 'main' into ejortegau/fix_tx_throttler_ignoring_initial_…

b83fc7b

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

ejortegau marked this pull request as draft March 15, 2023 09:38

Fix some CI pipeline issues

e33d173

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

ejortegau marked this pull request as ready for review March 16, 2023 12:39

ajm188 reviewed Mar 16, 2023

View reviewed changes

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

shlomi-noach reviewed Mar 16, 2023

View reviewed changes

shlomi-noach added Type: Bug Component: TabletManager and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 16, 2023

ejortegau requested review from shlomi-noach and removed request for deepthi, harshit-gangal, rohit-nayak-ps and systay March 20, 2023 08:43

Address PR comment.

fa04107

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

ejortegau removed the request for review from shlomi-noach March 20, 2023 12:41

ejortegau requested review from ajm188 and shlomi-noach and removed request for ajm188 March 20, 2023 12:41

timvaillancourt reviewed Mar 23, 2023

View reviewed changes

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

Fix typo

3d22c19

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

harshit-gangal approved these changes Mar 24, 2023

View reviewed changes

shlomi-noach approved these changes Mar 29, 2023

View reviewed changes

shlomi-noach merged commit 39f83b9 into vitessio:main Mar 29, 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 7, 2024

slack-vitess-r15.0.5: backport Transaction Throttler PRs, pt. 1 slackhq/vitess#335

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transaction throttler ignoring the initial rate#12618

Fix transaction throttler ignoring the initial rate#12618
shlomi-noach merged 8 commits intovitessio:mainfrom
ejortegau:ejortegau/fix_tx_throttler_ignoring_initial_rate

ejortegau commented Mar 13, 2023

Uh oh!

vitess-bot bot commented Mar 13, 2023 •

edited by shlomi-noach

Loading

Uh oh!

Uh oh!

shlomi-noach Mar 16, 2023 •

edited

Loading

Uh oh!

ejortegau Mar 20, 2023

Uh oh!

Uh oh!

shlomi-noach left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ejortegau commented Mar 13, 2023

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Mar 13, 2023 • edited by shlomi-noach Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

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

Uh oh!

Uh oh!

shlomi-noach Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejortegau Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vitess-bot bot commented Mar 13, 2023 •

edited by shlomi-noach

Loading

shlomi-noach Mar 16, 2023 •

edited

Loading