TxThrottler: dont throttle unless lag by ejortegau · Pull Request #14789 · vitessio/vitess

ejortegau · 2023-12-15T10:32:36Z

Description

This PR addresses issue #14768. It does so by ensuring that the transaction throttler checks for current lag in the shard before deciding whether to throttle the request or not.

The issue mentioned above is a bug causing artificially high throttling when the TxThrottler is enabled. Backporting of this is desirable as it will correct that undesirable behaviour in previous versions, not only in the next one. However, it seems I cannot add the backport to label to the PR.

Related Issue(s)

#14768

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

N/A

vitess-bot · 2023-12-15T10:32:38Z

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

codecov · 2024-01-18T11:03:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@6b481a7). Click here to learn what that means.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14789   +/-   ##
=======================================
  Coverage        ?   39.75%           
=======================================
  Files           ?     1639           
  Lines           ?   380078           
  Branches        ?        0           
=======================================
  Hits            ?   151084           
  Misses          ?   212777           
  Partials        ?    16217

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

go/vt/throttler/throttler.go

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

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

go/vt/throttler/throttler.go

deepthi

One more comment that needs to be addressed and then we can merge this. Thank you for your patience with the prolonged review process on this PR.

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

frouioui · 2024-02-09T17:20:40Z

It feels like this should have been backported to release-19.0. Adding the label now and re-triggering the event manually.

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: Florent Poinsard <florent.poinsard@outlook.fr>

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-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. 3 + ci fixes (#351) * txthrottler: add metrics for topoWatcher and healthCheckStreamer (vitessio#13153) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Replace deprecated `github.com/golang/mock` with `go.uber.org/mock` (vitessio#13512) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Per workload TxThrottler metrics (vitessio#13526) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * tx throttler: healthcheck all cells if `--tx-throttler-healthcheck-cells` is undefined (vitessio#12477) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Add dry-run/monitoring-only mode for TxThrottler (vitessio#13604) 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`: remove `txThrottlerConfig` struct, rely on `tabletenv` (vitessio#13624) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * tx throttler: remove unused topology watchers (vitessio#14412) Signed-off-by: deepthi <deepthi@planetscale.com> * tx_throttler: delete topo watcher metric instead of deprecating (vitessio#14445) Signed-off-by: deepthi <deepthi@planetscale.com> * TxThrottler: dont throttle unless lag (vitessio#14789) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * go test -v Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * add mutex to flaky parseFlags() Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * revert tweaks for flaky tests Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * fix protojson err Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * make vtadmin_web_proto_types Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * remove debug t.Logf(...) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> 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: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com> Signed-off-by: deepthi <deepthi@planetscale.com> Co-authored-by: Eng Zer Jun <engzerjun@gmail.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: Deepthi Sigireddi <deepthi@planetscale.com>

github-actions bot added this to the v19.0.0 milestone Dec 15, 2023

ejortegau force-pushed the ejortegau/txthrottler_dont_throttle_unless_lag branch from 7979573 to 231c2f6 Compare December 15, 2023 10:34

ejortegau marked this pull request as ready for review December 15, 2023 10:43

ejortegau requested review from deepthi, harshit-gangal, rohit-nayak-ps, shlomi-noach and systay as code owners December 15, 2023 10:43

ejortegau marked this pull request as draft December 15, 2023 11:08

ejortegau marked this pull request as ready for review December 18, 2023 16:24

harshit-gangal reviewed Jan 9, 2024

View reviewed changes

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

shlomi-noach removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Jan 16, 2024

harshit-gangal added Type: Bug Component: Query Serving labels Jan 16, 2024

deepthi changed the title ~~Ejortegau/txthrottler dont throttle unless lag~~ TxThrottler: dont throttle unless lag Jan 16, 2024

ejortegau requested a review from harshit-gangal January 18, 2024 15:12

harshit-gangal reviewed Jan 24, 2024

View reviewed changes

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

harshit-gangal approved these changes Jan 24, 2024

View reviewed changes

harshit-gangal reviewed Jan 24, 2024

View reviewed changes

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

ejortegau force-pushed the ejortegau/txthrottler_dont_throttle_unless_lag branch from 276a9f8 to 3c2acbb Compare January 24, 2024 13:13

ejortegau requested review from GuptaManan100 and frouioui as code owners January 24, 2024 13:13

deepthi added Backport to: release-17.0 and removed NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jan 31, 2024

deepthi reviewed Jan 31, 2024

View reviewed changes

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

ejortegau added 2 commits January 31, 2024 11:19

Address PR comments

57797ef

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

Address More PR comments

fde32b8

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

ejortegau requested a review from deepthi February 5, 2024 09:09

frouioui modified the milestones: v19.0.0, v20.0.0 Feb 6, 2024

deepthi reviewed Feb 8, 2024

View reviewed changes

go/vt/throttler/throttler.go Outdated Show resolved Hide resolved

deepthi approved these changes Feb 8, 2024

View reviewed changes

Address PR comments

80bdff5

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

deepthi merged commit 2b25639 into vitessio:main Feb 9, 2024

This was referenced Feb 9, 2024

[release-16.0] TxThrottler: dont throttle unless lag (#14789) #15188

Closed

[release-17.0] TxThrottler: dont throttle unless lag (#14789) #15189

Merged

vitess-bot bot mentioned this pull request Feb 9, 2024

[release-18.0] TxThrottler: dont throttle unless lag (#14789) #15190

Merged

frouioui added the Backport to: release-19.0 label Feb 9, 2024

vitess-bot pushed a commit that referenced this pull request Feb 9, 2024

TxThrottler: dont throttle unless lag (#14789)

c7c902a

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

vitess-bot bot mentioned this pull request Feb 9, 2024

[release-19.0] TxThrottler: dont throttle unless lag (#14789) #15196

Merged

frouioui pushed a commit that referenced this pull request Feb 9, 2024

TxThrottler: dont throttle unless lag (#14789)

91de40c

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

timvaillancourt pushed a commit to slackhq/vitess that referenced this pull request May 16, 2024

TxThrottler: dont throttle unless lag (vitessio#14789)

bb2c0e1

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

timvaillancourt mentioned this pull request May 16, 2024

slack-vitess-r15.0.5: backport required Transaction Throttler PRs, pt. 3 + ci fixes slackhq/vitess#351

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TxThrottler: dont throttle unless lag#14789

TxThrottler: dont throttle unless lag#14789
deepthi merged 13 commits intovitessio:mainfrom
ejortegau:ejortegau/txthrottler_dont_throttle_unless_lag

ejortegau commented Dec 15, 2023 •

edited by deepthi

Loading

Uh oh!

vitess-bot bot commented Dec 15, 2023

Uh oh!

Uh oh!

codecov bot commented Jan 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deepthi left a comment

Uh oh!

frouioui commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ejortegau commented Dec 15, 2023 • edited by deepthi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Dec 15, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Uh oh!

Uh oh!

codecov bot commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deepthi left a comment

Choose a reason for hiding this comment

Uh oh!

frouioui commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ejortegau commented Dec 15, 2023 •

edited by deepthi

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading