Skip to content

DRE-8939 Backport TxThrottler dry run mode#13687

Closed
ejortegau wants to merge 217 commits intovitessio:mainfrom
slackhq:ejortegau/slack-vitess-r14.0.5-dsdefense-tx-throttler-dry-run-mode
Closed

DRE-8939 Backport TxThrottler dry run mode#13687
ejortegau wants to merge 217 commits intovitessio:mainfrom
slackhq:ejortegau/slack-vitess-r14.0.5-dsdefense-tx-throttler-dry-run-mode

Conversation

@ejortegau
Copy link
Copy Markdown
Contributor

Description

This adds the ability to run the vttablet transaction throttler in dry run mode. It is a backport of #13604

Related Issue(s)

https://jira.tinyspeck.com/browse/DRE-8938

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

frouioui and others added 30 commits June 7, 2022 16:24
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
* docs: added to the release notes

Signed-off-by: Andres Taylor <andres@planetscale.com>

* Add name to static check workflow

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Co-authored-by: Rohit Nayak <rohit@planetscale.com>
…vitessio#10471) (vitessio#10474)

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Andrew Mason <andrew@planetscale.com>
…0453) (vitessio#10466)

* fix: change planner_version to planner-version everywhere

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: actually change the planner version on vtgate after checking

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: move CheckPlannerVersionFlag out from vtgate

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: move the global plannerVersion to be a field on the executor. think global, act local, yo

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: use the planner in the session first

Signed-off-by: Andres Taylor <andres@planetscale.com>

* test: use DEFAULT instead of 0

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: re-add the planner-version flag to vtcombo

Signed-off-by: Andres Taylor <andres@planetscale.com>
* Add PingTablet to mock vtctld

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for PingTablet

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for RefreshState

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for RefreshTabletReplicationSource

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add RunHealthCheck to mock vtctld

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for RunHealthCheck

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for SetReadOnly

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for SetReadWrite

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for StartReplication

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for StopReplication

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for TabletExternallyPromoted

Signed-off-by: Andrew Mason <andrew@planetscale.com>
* add sharding column name/type deprecation to release notes summary

Signed-off-by: deepthi <deepthi@planetscale.com>

* deprecation: mark sharding-column-name and sharding-column-type for CreateKeyspace command as deprecated

Signed-off-by: deepthi <deepthi@planetscale.com>
* [vtadmin] Add authz tests for remaining non-schema related actions (vitessio#10481)

* Add authz tests for EmergencyFailoverShard

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for PlannedFailoverShard

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add ValidateKeyspace to mock vtctld

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for ValidateKeyspace

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add ValidateSchemaKeyspace to mock vtctld

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for ValidateSchemaKeyspace

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add ValidateVersionKeyspace to mock vtctld

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for ValidateVersionKeyspace

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* fixup mock data sorting

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* [vtadmin] Add schema-related authz tests (vitessio#10486)

* Update template to support more tablet fields

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update template, add full srvvschema payload, add schema payload, add authz tests for VTExplain

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for GetSchema

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for GetSchemas

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for FindSchema

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Add authz tests for ReloadSchemas

Signed-off-by: Andrew Mason <andrew@planetscale.com>
…io#10502) (vitessio#10509)

* Update docs for backup commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for cell commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for keyspace commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for legacy-shim command

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs, add aliases for reparent commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for root command

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for routing rule commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for schema commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for serving graph commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for shard commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for tablet commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for vschema commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Update docs for workflow commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Remove references to deprecated flags

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* fixup! Update docs for keyspace commands

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Remove reparent command aliases

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* Specify that `--server` is required

Signed-off-by: Andrew Mason <andrew@planetscale.com>
… and move it to places which will only be called less frequently (vitessio#10516) (vitessio#10518)

Signed-off-by: Manan Gupta <manan@planetscale.com>
… and vitessio#10514 (vitessio#10517)

* Fix parsing of CAST() statements (vitessio#10512)

* Fix parsing of CAST() statements

CAST() was treated as an alias for CONVERT() but with slightly different
syntax.

This is also described in the documentation at
https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html,
specifically:

With CAST(expr AS type syntax, the CAST() function takes an expression of
any type and produces a result value of the specified type. This operation
may also be expressed as CONVERT(expr, type), which is equivalent. If expr
is NULL, CAST() returns NULL.

This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically
in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is
possible on a CAST(), but that is not accepted for a CONVERT().

The difference in parsing also shows in MySQL:

```
mysql> select convert(json_keys(c), char(64) array) from t;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1
mysql> select cast(json_keys(c) as char(64) array) from t;
ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions'
```

Here the first statement can't be parsed at all. The second is properly
parsed, but ARRAY is not allowed in the context of CAST() in this
situation (and only in a CREATE TABLE).

This means we should really treat these as two separate expressions and
don't store them both in the same structure. The change here creates a
separate CAST structure, removes the ARRAY option from CONVERT and
updates the grammar and all tests accordingly.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* Handle new cast expression in evalengine and planbuilder

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* evalengine: do not duplicate CAST/CONVERT translation

Signed-off-by: Vicent Marti <vmg@strn.cat>

Co-authored-by: Vicent Marti <vmg@strn.cat>
Signed-off-by: Manan Gupta <manan@planetscale.com>

* Add back unary single column expression check (vitessio#10514)

This was accidentally removed in
vitessio#10512 but it shouldn't have
been.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
Co-authored-by: Vicent Marti <vmg@strn.cat>
…#10508) (vitessio#10528)

* addition of the release_notes_label workflow

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* addition of a helper step in the release_notes_label to guide users

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* addition of a set of rules for release_notes_labels's pull_request target

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* remove the previous workflow that used to check the release notes labels

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Change the review check list and enforce the use of type/component labels

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Fixed syntax in check release notes label workflow

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
…o#10484)

Signed-off-by: Andrew Mason <andrew@planetscale.com>
…#10532)

* generate the release changelog as a second document

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* include every PR in the release notes

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Moved changelog metrics away from the changelog and added summary for v15

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
…0553)

* test: added failing e2e test

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* fix: handle empty row for scalar aggregation and also olap engine for scalar aggregation

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: update plan test output

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: e2e test for empty rows in non-scalar aggregates

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>

Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
* docs: add blurb about gen4 being the new planner

Signed-off-by: Andres Taylor <andres@planetscale.com>

* docs: release notes

Signed-off-by: Andres Taylor <andres@planetscale.com>
…itessio#10464) (vitessio#10546)

* Adds RPCs to vttablet that vtorc requires (vitessio#10464)

* feat: add vttablet rpc to reset replication parameters

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: added end to end testing for the rpc and fixed bug

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: fix typing error

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add basic full status rpc functionality and add test for it

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add all the fields needed in full status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: moved the test to reparent tests and improved it

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: bug fix for no replication status and no primary status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add version to the full status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add binlog information to full status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* docs: fix the comment explaining the binlog information

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add semi-sync statuses to full status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: call the correct command

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add server uuid and id to full status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: make server_id a uint32 to accept the correct range of values

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add few more fields to the full status like version comment, semi-sync settings, binlog_row_image

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: generate vtadmin proto files

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add assertion to check binlog row format is read correctly

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: split GTID mode in its own function because mariadb doesn't support it

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: fix parsing of empty mariadb gtid set

Signed-off-by: Manan Gupta <manan@planetscale.com>

* docs: add doucmentation for existing fields in ReplicationStatus

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add relay log file position to the replication status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: augmented full status test to check all the different positions stored

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add additional fields to replication status and read source user

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: read sql delay from show replica status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: read ssl allowed from show replica status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: read has replication filters from show replica status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: read auto position and using gtid from show replica status output

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add replication lag unknown too to replication status

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: return nils from replication and primary postiion if it is not present

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: rename FileRelayLogPosition in replication status output to RelayLogSourceBinLogEquivalentPosition and augment test to make sure rpc changes are backward compatible

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: update vtadmin proto files

Signed-off-by: Manan Gupta <manan@planetscale.com>

* refactor: rename BinLog to binlog in renamed proto field

Signed-off-by: Manan Gupta <manan@planetscale.com>

* lint: add a new line to proto file

Signed-off-by: Manan Gupta <manan@planetscale.com>
…io#10562)

* VReplication: more unrecoverable error codes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* correct path for test files

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
* test: reproduce the panic as a unit test

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: check ev is not nil before using its fields

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: increase timeout of LockShard and wait replicas in VTOrc default config

Signed-off-by: Manan Gupta <manan@planetscale.com>
…#10535) (vitessio#10570)

The canonical form of printing the vitess migration syntax would upcase
the UUID value, but it needs to be passed through unchanged.

This in turn led me to testing comments in a query as well and it turns
out we wrongly change the case there as well.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
…sist and unrecoverable errors (vitessio#10573)

* Fail VReplication workflows on errors that persist and unrecoverable errors (vitessio#10429)

* Fail workflow if same error persists too long. Fail for unrecoverable errors also in non-online ddl workflows

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* Update max time default to 15m, was 1m for testing purposes

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* Leverage vterrors for Equals; attempt to address my own nits

Signed-off-by: Matt Lord <mattalord@gmail.com>

* sanity: validate range of vreplication_retry_delay and of vreplication_max_time_to_retry_on_error

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* Fix flags test

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* Remove leftover log.Flush()

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* Revert validations min/max settings on retry delay since it is breaking unit tests that set the value to a very small value

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* captilize per request

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Co-authored-by: Matt Lord <mattalord@gmail.com>
Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* fix TestHelpOutput

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* spaces, not tabs

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Co-authored-by: Rohit Nayak <57520317+rohit-nayak-ps@users.noreply.github.com>
Co-authored-by: Matt Lord <mattalord@gmail.com>
…sts (vitessio#10522)

* Take into account `github.ref` when doing upgrade-downgrade tests (vitessio#10504)

* Take into account the github.ref variable when doing upgrade-downgrade tests

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Changed upgrade-downgrade workflows' syntax to use 'previous' instead of 'latest'

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Fixed syntax error in backup upgrade downgrade workflow

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Fixed static_checks_etc workflow

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
…hannel might block the vstream thread if target channel goes away: context was not being checked then. Fix health stream goroutine leak. (vitessio#10576)

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
* add-vtadmin-docker-image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* env as function, update tests and code

Signed-off-by: Andrew Mason <andrew@planetscale.com>
Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* feat: fix remaining env usages to be function calls

Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* add vtadmin build result in the bootstrap image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* fix vtadmin web cleaning

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* add vtadmin docker image entrypoint

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* add the possibility to custom vtadmin web port in Docker image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* fix vtadmin docker port

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* fix vtadmin entrypoint

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* set default vtadmin docker user as vitess

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* do not build vtadmin frontend in the bootstrap image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* build vtadmin frontend only in vtadmin docker image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* vtadmin replace sed and custom entrypoint with config.js file

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* remove vitess web files in lite docker image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* move vtadmin config into a specific directory

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

* restore vtadmin web file in mysql57 lite docker image

Signed-off-by: Léopold Jacquot <leopold.jacquot@infomaniak.com>

Co-authored-by: Andrew Mason <andrew@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>

Co-authored-by: Léopold Jacquot <leopold.jacquot@gmail.com>
Co-authored-by: Andrew Mason <andrew@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: deepthi <deepthi@planetscale.com>
* CherryPick: VDiff2: Support Resuming VDiffs (vitessio#10497)

And fix a number of bugs discovered related to incorrect VDiff summary handling and other more minor things.

Signed-off-by: Matt Lord <mattalord@gmail.com>
tanjinx and others added 23 commits May 9, 2023 14:09
…ES (vitessio#12949) (#81)

This error code seems better suited to represent the fact that transactions are
being throttled by the server due to some form of resource contention than the
current code 1203 ER_TOO_MANY_USER_CONNECTIONS.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.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>

* Fix bad conflict resolution

---------

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

* Implement the Vitess Structure Logger VTSLoger

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Downgrade zap from 1.24.0 to 1.23.0

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Ran go mod tidy

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

* Fix help text test

Signed-off-by: Emad Habib <ehabib@slack-corp.com>

---------

Signed-off-by: Emad Habib <ehabib@slack-corp.com>
…itessio#12901 (#83)

* 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>

* remove unused cell string

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
* Add vstream metrics to vtgate



* Update unit test name and use cell variable



* Reset metrics for TestVStreamsCreatedAndLagMetrics, fix data race issue



---------

Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>
Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
* 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>
…3040)

* 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>
Signed-off-by: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com>
* txthrottler: further code cleanup



* Fix bad merge resolution



---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…let type (vitessio#12174) (#95)

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

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Remove mistaken git add

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Priya Bibra <pbibra@slack-corp.com>
Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com>
This is a backport of upstreamed vitessio#13526

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
This is a backport of upstreamed vitessio#13604

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot bot commented Aug 2, 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.

@vitess-bot vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Aug 2, 2023
@ejortegau ejortegau closed this Aug 2, 2023
@github-actions github-actions bot added this to the v18.0.0 milestone Aug 2, 2023
ejortegau added a commit to slackhq/vitess that referenced this pull request Aug 3, 2023
* BackportAdd dry-run mode to the TxThrottler

This is a backport of upstreamed vitessio#13604

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says

Projects

None yet

Development

Successfully merging this pull request may close these issues.