Skip to content
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: further code cleanup #12902

Merged

Conversation

timvaillancourt
Copy link
Contributor

@timvaillancourt timvaillancourt commented Apr 15, 2023

Description

Building on top of #12901, this PR cleans up go/vt/vttablet/tabletserver/txthrottler by:

  1. Reordering structs based on @deepthi's recommendation in Cleanup panics in txthrottler, reorder for readability #12901:

    I would move the structs up to the beginning AND reorder the struct methods so that state and config methods are defined right after the struct definition of TxThrottler. Such wholesale reorganization will definitely make the PR harder to review, so you might want to get the panic changes all reviewed and squared away and then do the re-org as a separate commit.

  2. Removing *topo.Server from txThrottlerConfig struct because *topo.Server is not really "configuration"
    • Added *topo.Server to TxThrottler struct and passed down instead
  3. Moving func init() to be after the func it calls: func resetTxThrottlerFactories()

Related Issue(s)

#12901

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

@github-actions github-actions bot added this to the v17.0.0 milestone Apr 15, 2023
@timvaillancourt timvaillancourt marked this pull request as draft May 2, 2023 19:51
@timvaillancourt timvaillancourt marked this pull request as ready for review May 3, 2023 15:25
@vitess-bot
Copy link
Contributor

vitess-bot bot commented May 3, 2023

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

@harshit-gangal
Copy link
Member

@timvaillancourt There is a conflict that needs to be resolved.

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@timvaillancourt
Copy link
Contributor Author

@harshit-gangal conflict resolved! 🙇

@harshit-gangal harshit-gangal merged commit e05a0b8 into vitessio:main May 17, 2023
@timvaillancourt timvaillancourt deleted the tx-throttler-cleanup-pt2 branch May 17, 2023 10:03
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Jun 28, 2023
* 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>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Jul 3, 2023
* txthrottler: further code cleanup



* Fix bad merge resolution



---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Apr 16, 2024
* 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>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request May 16, 2024
* 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>
timvaillancourt added a commit to slackhq/vitess that referenced this pull request May 16, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants