slack-vitess-r15.0.5: backport required Transaction Throttler PRs, …

…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 <[email protected]>

* Remove unused variable

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix CI pipeline

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Address PR comments.

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Make linter happy & add extra test cases.

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Address PR comments.

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix circular import

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Make linter happy

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Address PR comments:

* Invalid criticality in query directive fails the query.

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* 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 <[email protected]>

* Add missing piece of code that got lost during merge conflict resolution

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix vtadmin.js

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Address PR comments

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix unit tests (I think)

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* 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 <[email protected]>

* Make linter happy

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix flag e2e test

Signed-off-by: Eduardo J. Ortega U <[email protected]>

---------

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Add flag to select tx throttler tablet type (vitessio#12174)

* Add flag to select tx throttler tablet type

Signed-off-by: Tim Vaillancourt <[email protected]>

* REPLICA and/or RDONLY only

Signed-off-by: Tim Vaillancourt <[email protected]>

* Update flag help msg

Signed-off-by: Tim Vaillancourt <[email protected]>

* Lowercase types in help/doc

Signed-off-by: Tim Vaillancourt <[email protected]>

* Help update

Signed-off-by: Tim Vaillancourt <[email protected]>

* fix test

Signed-off-by: Tim Vaillancourt <[email protected]>

* No underscores in flag

Signed-off-by: Tim Vaillancourt <[email protected]>

* Fix test

Signed-off-by: Tim Vaillancourt <[email protected]>

* Fix merge

Signed-off-by: Tim Vaillancourt <[email protected]>

* PR suggestion, consolidate config logic

Signed-off-by: Tim Vaillancourt <[email protected]>

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

Co-authored-by: Andrew Mason <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>

* Use topoproto.TabletTypeListFlag to handle flag

Signed-off-by: Tim Vaillancourt <[email protected]>

* Fix unit test

Signed-off-by: Tim Vaillancourt <[email protected]>

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

Co-authored-by: Andrew Mason <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>

* improve test

Signed-off-by: Tim Vaillancourt <[email protected]>

* pr suggestions

Signed-off-by: Tim Vaillancourt <[email protected]>

* go fmt

Signed-off-by: Tim Vaillancourt <[email protected]>

---------

Signed-off-by: Tim Vaillancourt <[email protected]>
Co-authored-by: Andrew Mason <[email protected]>

* txthrottler: further code cleanup (vitessio#12902)

* txthrottler: further code cleanup

Signed-off-by: Tim Vaillancourt <[email protected]>

* Fix bad merge resolution

Signed-off-by: Tim Vaillancourt <[email protected]>

---------

Signed-off-by: Tim Vaillancourt <[email protected]>

* 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 <[email protected]>

* Fix e2e flag tests & TxThrottler unit test

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Add note on new TxThrottler behavior in v17 changelog

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Fix PR number in changelog entry for TxThrottler behavior change.

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Make linter happy

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* Address PR comments

Signed-off-by: Eduardo J. Ortega U <[email protected]>

---------

Signed-off-by: Eduardo J. Ortega U <[email protected]>

* txthrottler: verify config at vttablet startup, consolidate funcs (vitessio#13115)

* txthrottler: verify config at vttablet startup, consolidate funcs

Signed-off-by: Tim Vaillancourt <[email protected]>

* Use explicit dest in prototext.Unmarshal

Signed-off-by: Tim Vaillancourt <[email protected]>

* Use for loop for TestVerifyTxThrottlerConfig

Signed-off-by: Tim Vaillancourt <[email protected]>

* Cleanup test

Signed-off-by: Tim Vaillancourt <[email protected]>

* Fix go vet complaint

Signed-off-by: Tim Vaillancourt <[email protected]>

* Add back synonym flag

Signed-off-by: Tim Vaillancourt <[email protected]>

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

Co-authored-by: Shlomi Noach <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>

* Address staticcheck linter error

Signed-off-by: Tim Vaillancourt <[email protected]>

---------

Signed-off-by: Tim Vaillancourt <[email protected]>
Co-authored-by: Shlomi Noach <[email protected]>

* gofumpt

Signed-off-by: Tim Vaillancourt <[email protected]>

---------

Signed-off-by: Eduardo J. Ortega U <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Co-authored-by: Eduardo J. Ortega U <[email protected]>
Co-authored-by: Andrew Mason <[email protected]>
Co-authored-by: Shlomi Noach <[email protected]>

config/tablet/default.yaml

@@ -117,6 +117,7 @@ cacheResultFields: true                  # enable-query-plan-field-caching
 # enable-tx-throttler
 # tx-throttler-config
 # tx-throttler-healthcheck-cells
+# tx-throttler-tablet-types
 # enable_transaction_limit
 # enable_transaction_limit_dry_run
 # transaction_limit_per_user

doc/ReplicationLagBasedThrottlingOfTransactions.md

@@ -30,7 +30,13 @@ If this is not specified a [default](https://github.com/vitessio/vitess/tree/mai
 * *tx-throttler-healthcheck-cells*
 
 A comma separated list of datacenter cells. The throttler will only monitor
-the non-RDONLY replicas found in these cells for replication lag.
+the replicas found in these cells for replication lag.
+
+* *tx-throttler-tablet-types*
+
+A comma separated list of tablet types. The throttler will only monitor tablets
+with these types. Only `replica` and/or `rdonly` types are supported. The default
+is `replica`.
 
 # Caveats and Known Issues
 * The throttler keeps trying to explore the maximum rate possible while keeping
@@ -39,4 +45,3 @@ lag limit may occasionally be slightly violated.
 
 * Transactions are considered homogeneous. There is currently no support
 for specifying how `expensive` a transaction is.
-

go/flags/endtoend/vttablet.txt

@@ -343,9 +343,11 @@ Usage of vttablet:
       --twopc_abandon_age float                                          time in seconds. Any unresolved transaction older than this time will be sent to the coordinator to be resolved.
       --twopc_coordinator_address string                                 address of the (VTGate) process(es) that will be used to notify of abandoned transactions.
       --twopc_enable                                                     if the flag is on, 2pc is enabled. Other 2pc flags must be supplied.
-      --tx-throttler-config string                                       Synonym to -tx_throttler_config (default "target_replication_lag_sec: 2\nmax_replication_lag_sec: 10\ninitial_rate: 100\nmax_increase: 1\nemergency_decrease: 0.5\nmin_duration_between_increases_sec: 40\nmax_duration_between_increases_sec: 62\nmin_duration_between_decreases_sec: 20\nspread_backlog_across_sec: 20\nage_bad_rate_after_sec: 180\nbad_rate_increase: 0.1\nmax_rate_approach_threshold: 0.9\n")
+      --tx-throttler-config string                                       Synonym to -tx_throttler_config (default "target_replication_lag_sec:2 max_replication_lag_sec:10 initial_rate:100 max_increase:1 emergency_decrease:0.5 min_duration_between_increases_sec:40 max_duration_between_increases_sec:62 min_duration_between_decreases_sec:20 spread_backlog_across_sec:20 age_bad_rate_after_sec:180 bad_rate_increase:0.1 max_rate_approach_threshold:0.9")
+      --tx-throttler-default-priority int                                Default priority assigned to queries that lack priority information (default 100)
       --tx-throttler-healthcheck-cells strings                           Synonym to -tx_throttler_healthcheck_cells
-      --tx_throttler_config string                                       The configuration of the transaction throttler as a text formatted throttlerdata.Configuration protocol buffer message (default "target_replication_lag_sec: 2\nmax_replication_lag_sec: 10\ninitial_rate: 100\nmax_increase: 1\nemergency_decrease: 0.5\nmin_duration_between_increases_sec: 40\nmax_duration_between_increases_sec: 62\nmin_duration_between_decreases_sec: 20\nspread_backlog_across_sec: 20\nage_bad_rate_after_sec: 180\nbad_rate_increase: 0.1\nmax_rate_approach_threshold: 0.9\n")
+      --tx-throttler-tablet-types strings                                A comma-separated list of tablet types. Only tablets of this type are monitored for replication lag by the transaction throttler. Supported types are replica and/or rdonly. (default replica)
+      --tx_throttler_config string                                       The configuration of the transaction throttler as a text-formatted throttlerdata.Configuration protocol buffer message. (default "target_replication_lag_sec:2 max_replication_lag_sec:10 initial_rate:100 max_increase:1 emergency_decrease:0.5 min_duration_between_increases_sec:40 max_duration_between_increases_sec:62 min_duration_between_decreases_sec:20 spread_backlog_across_sec:20 age_bad_rate_after_sec:180 bad_rate_increase:0.1 max_rate_approach_threshold:0.9")
       --tx_throttler_healthcheck_cells strings                           A comma-separated list of cells. Only tabletservers running in these cells will be monitored for replication lag by the transaction throttler.
       --unhealthy_threshold duration                                     replication lag after which a replica is considered unhealthy (default 2h0m0s)
       --use_super_read_only                                              Set super_read_only flag when performing planned failover.

go/flagutil/flagutil.go

@@ -193,6 +193,22 @@ func DualFormatBoolVar(fs *pflag.FlagSet, p *bool, name string, value bool, usag
 	}
 }
 
+// DualFormatVar creates a flag which supports both dashes and underscores
+func DualFormatVar(fs *pflag.FlagSet, val pflag.Value, name string, usage string) {
+	dashes := strings.Replace(name, "_", "-", -1)
+	underscores := strings.Replace(name, "-", "_", -1)
+
+	fs.Var(val, underscores, usage)
+	if dashes != underscores {
+		fs.Var(val, dashes, fmt.Sprintf("Synonym to -%s", underscores))
+	}
+}
+
+type Value[T any] interface {
+	pflag.Value
+	Get() T
+}
+
 // DurationOrIntVar implements pflag.Value for flags that have historically been
 // of type IntVar (and then converted to seconds or some other unit) but are
 // now transitioning to a proper DurationVar type.