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

Feature Request: support throttling on "any" cell by default in vttablet transaction throttler #12435

Closed
timvaillancourt opened this issue Feb 22, 2023 · 5 comments · Fixed by #12477

Comments

@timvaillancourt
Copy link
Contributor

timvaillancourt commented Feb 22, 2023

Feature Description

Currently the vttablet transaction throttler will not start if the list of cells to healthcheck is not defined. This scenario also causes the transaction throttler to be somewhat-silently disabled, which is another issue I reported here

An example:

$ /mnt/vitess/bin/vitess/current/vttablet -tablet-path us_east_1e-123456 \
          -mycnf-file /mnt/vitess/mysql/datadir/my.cnf -topo_implementation "consul" \
          -topo_global_server_address "consul.redacted.com:8500" -topo_global_root "vitess/global" \
          -init_keyspace test -init_shard -80 -init_tablet_type "replica" \
          -enable-tx-throttler 
E0221 10:13:18.843054    6307 tx_throttler.go:88] Error creating transaction throttler. Transaction throttling will be disabled. Error: empty healthCheckCells given. &{enabled:true topoServer:0xc0012319c0 throttlerConfig:0xc001217950 healthCheckCells:[]}
W0221 10:13:18.970389    6307 tm_init.go:706] creating keyspace and shard failed (Get "http://consul.redacted.com:8500/v1/kv/vitess/global/keyspaces/test/shards/-80/Shard": dial tcp: lookup consul.redacted.com on 127.0.0.1:53: no such host), backing off 1s before retrying
W0221 10:13:19.973236    6307 tm_init.go:706] creating keyspace and shard failed (Get "http://consul.redacted.com:8500/v1/kv/vitess/global/keyspaces/test/shards/-80/Shard": dial tcp: lookup consul.redacted.com on 127.0.0.1:53: no such host), backing off 1.125602181s before retrying
W0221 10:13:21.101325    6307 tm_init.go:706] creating keyspace and shard failed (Get "http://consul.redacted.com:8500/v1/kv/vitess/global/keyspaces/test/shards/-80/Shard": dial tcp: lookup consul.redacted.com on 127.0.0.1:53: no such host), backing off 1.295018994s before retrying
^C

When no -tx-throttler-healthcheck-cells is specified I suggest vttablet uses all available cells instead of refusing to start the transaction throttler

I am happy to make a PR implementing what is decided here 👍. If we decide to leave this as-is, I feel more could be done to highlight that -tx-throttler-healthcheck-cells is a required flag when using -enable-tx-throttler

Use Case(s)

A fallback to "any cell" simplifies deployment/config automation for some users who today need to define an exhaustive list of cells to monitor. Users without a preference on cells to monitor can simply not-define this flag.

To me, this seems like the more intuitive default as well.

@timvaillancourt timvaillancourt added Needs Triage This issue needs to be correctly labelled and triaged Type: Feature labels Feb 22, 2023
@timvaillancourt timvaillancourt changed the title Feature Request: support throttling on "any" cell in vttablet transaction throttler Feature Request: support throttling on "any" cell by default in vttablet transaction throttler Feb 22, 2023
@harshit-gangal
Copy link
Member

@timvaillancourt are you suggesting all cells or any cells? Use case mentions any while descriptions says all

@timvaillancourt
Copy link
Contributor Author

@harshit-gangal I mean all cells available to the vttablet assuming that is easy to know

For example: if deployed in a cluster with cells dc1, dc2 and dc3, the vttablet Transaction Throttler will consider healthchecks from dc1, dc2 and dc3 when --tx-throttler-healthcheck-cells is not provided

Today the transaction throttler will fail to start unless an explicit list of cells is provided, even if you just want "all available"

@harshit-gangal
Copy link
Member

I like this. Thank you for clarifying.

@harshit-gangal harshit-gangal removed the Needs Triage This issue needs to be correctly labelled and triaged label Feb 23, 2023
@timvaillancourt
Copy link
Contributor Author

timvaillancourt commented Feb 24, 2023

@harshit-gangal thanks! I think it's more in line with how --cells-to-watch and other 'selectors' work 👍

A PR has been created to implement this: #12477

@shlomi-noach
Copy link
Contributor

I'm also in agreement this should be the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants