Backport #13130: Tablet throttler: throttler-config-via-topo defaults 'true', deprecation message for old flags #13237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ion message for old flags (vitessio#13130) * Table throttler: --throttler-config-via-topo now defaults to 'true' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * add deprecation message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * endtoend tests: remove '--enable-lag-throttler' and use 'UpdateThrottlerConfig' everywhere Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * always use vtctldclient Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * use cluster.VtctldClientProcess Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * disable --throttler-config-via-topo in old throttler tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Remove --throttler-config-via-topo where used, since it now defaults 'true' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * fix vreplication cluster setup, waiting for throttler config to apply Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * changelog Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * extend throttler threshold Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * a bit more verbose Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * fixed CLI test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * remove old '--enable-lag-throttler' flag, introduce '--heartbeat_on_demand_duration' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * more log info in throttler.Open() Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * more logging Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Revert to --heartbeat_enable Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Protect throttler config change application with initMutex And in e2e test update the throttler config on the keyspace when it's created. Only wait for the new tablets in a shard to have the throttler enabled when adding a Shard. Signed-off-by: Matt Lord <mattalord@gmail.com> * More CI testing Signed-off-by: Matt Lord <mattalord@gmail.com> * CI testing cont Signed-off-by: Matt Lord <mattalord@gmail.com> * Yes... Signed-off-by: Matt Lord <mattalord@gmail.com> * Somebody doesn't like force pushes so msg here Signed-off-by: Matt Lord <mattalord@gmail.com> * Increase on-demand heartbeat duration from 10s to 1m Signed-off-by: Matt Lord <mattalord@gmail.com> * Use only on-demand heartbeats everywhere Signed-off-by: Matt Lord <mattalord@gmail.com> * Use same throttler config everywhere Signed-off-by: Matt Lord <mattalord@gmail.com> * Update all keyspaces and don't fail test on missing JSON keys Signed-off-by: Matt Lord <mattalord@gmail.com> * Use constant heartbeats in vrepl e2e tests Until vitessio#13175 is fixed. Signed-off-by: Matt Lord <mattalord@gmail.com> * Increase workflow command timeout Signed-off-by: Matt Lord <mattalord@gmail.com> * Don't wait for throttler on non-serving primaries Signed-off-by: Matt Lord <mattalord@gmail.com> * vitessio#13175 is fixed, therefore re-instating on-deman heartbeats Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Added ToC Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Tweak comment and kick CI Signed-off-by: Matt Lord <mattalord@gmail.com> * Treat isOpen as the ready/running signal. Also align all initMutex usage. Signed-off-by: Matt Lord <mattalord@gmail.com> * Re-adjust comment Signed-off-by: Matt Lord <mattalord@gmail.com> * Adjust CheckIsReady() to match OnlineDDL's expectation/usage This was only using IsReady() before, now it's using IsOpen() and IsReady(). Signed-off-by: Matt Lord <mattalord@gmail.com> * Get rid of log messages from SrvKeyspaceWatcher when no node/key Signed-off-by: Matt Lord <mattalord@gmail.com> * More corrections/tweaks Signed-off-by: Matt Lord <mattalord@gmail.com> * Use more convenient/clear new IsRunning function Signed-off-by: Matt Lord <mattalord@gmail.com> * Revert "Use more convenient/clear new IsRunning function" This reverts commit 9aef276 as this change was not correct. Signed-off-by: Matt Lord <mattalord@gmail.com> * Further fix correct use of IsOpen(), IsRunning(), IsEnabled() Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * throttler.throttledApps cannot be nil Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * minor refactory/beautify for test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * fix flakiness of tabletmanager_throttler_topo test by: (1) proper wait-for functions, and (2) issue different queries per goroutine Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Fix typo in release notes Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>
shlomi-noach
requested review from
harshit-gangal,
systay,
rohit-nayak-ps,
mattlord and
deepthi
as code owners
vitess-bot
bot
added
NeedsDescriptionUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. 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
|
shlomi-noach
added
Backport
mattlord
approved these changes
Jun 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #13130
Description
v16introduced a newvttabletflag:--throttler-config-via-topo, see the docs: https://vitess.io/docs/16.0/reference/features/tablet-throttler/v16this flag defaultsfalse, and the old per-tablet--enable-lag-throttlerconfiguration is still supported.v17, the target of this PR, this PR sets the default for--throttler-config-via-topototrue. The old configuration is still supported but if used there's a deprecation warning.v18, the old configuration & logic will be compeletely removed and it will be assumed that--throttler-config-via-topois alwaystruewhether specified or not. The flag will issue a deprecation message.v19we will remove the flag--throttler-config-via-topo.We remove all references to
--enable-lag-throttlerin Vitess's ownendtoendtests, and use the dynamic throttler config everywhere.Related Issue(s)
Follow up to:
Checklist