Add flag to select tx throttler tablet type

[timvaillancourt]

Description

This PR allows the user to specify which tablet type(s) should be monitored by the vttablet Transaction Throttler (Design Doc). The motivation for this is to allow rdonly tablets to be included in throttling

This involves the addition of the --tx-throttler-tablet-types flag (a comma-separated list of tablet types) with the default of replica for backwards-compatability. Only replica and/or rdonly are supported

This resolves this code comment in tx_throttler.go:

// TODO(erez): If this becomes necessary, we can add a configuration option that would
// determine whether we consider RDONLY tablets here, as well.

vttablet --help output:

tim@Tims-MacBook-Pro vitess % ./vttablet --help 2>&1 | grep tx-throttler-tablet-types
      --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)

Backports to v14 + v15 would be much appreciated if they merge cleanly

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Documentation was added or is not required

Deployment Notes

[deepthi]

Looks like our bot isn't running right now, otherwise there would have been a comment like this #12138 (comment)

Relevant points:

• New flags should use dashes, not underscores
• New flags need to be documented on the website

Re backports, we do them only for regressions / production blocking bugs. This doesn't seem like either. If that isn't right and you can provide a justification for the back port, I'm happy to discuss.

[timvaillancourt]

@deepthi thanks for the feedback, updating the PR based on this 👍

• New flags should use dashes, not underscores

The majority of the existing tx throttler flags are using underscores, would it make sense for me to update those also or is an inconsistency acceptable?

Re backports, we do them only for regressions / production blocking bugs. This doesn't seem like either.

Good point. This does not meet the criteria for backporting 👍

[timvaillancourt]

The majority of the existing tx throttler flags are using underscores, would it make sense for me to update those also or is an inconsistency acceptable?

For now I have updated the new flag only to dashes. It appears the existing flags have dash-based aliases with help like: Synonym to -tx_throttler_healthcheck_cells 👍

[timvaillancourt]

Website docs PR: vitessio/website#1368

[deepthi]

This looks pretty good to me. The only thing I'd ask is to add a unit test that actually tests with multiple tablet types.
e2e tests are expensive so I'm not going to ask for one. Can you confirm that you are able to run this locally and get the expected behavior?
Website PR is looking good.

[deepthi]

The majority of the existing tx throttler flags are using underscores, would it make sense for me to update those also or is an inconsistency acceptable?

For now I have updated the new flag only to dashes. It appears the existing flags have dash-based aliases with help like: Synonym to -tx_throttler_healthcheck_cells 👍

You did the right thing. We eventually want to move all flags to be only dashes, but we have a long way to go until we get there. It will take a couple more releases (at least).

[timvaillancourt]

@ajm188 good catches thanks, added those suggestions 👍

[harshit-gangal]

I have a question here.

The purpose of the tx throttler is to avoid creating new transactions as the existing replicas are lagging and the new transactions would not be able to commit as they would not receive semi-sync acks.

What purpose we would solve to throttler on rdonly tablet types. primary tablets do not expect semi-sync acks from those tablets. We would be throttling user traffic when they can actually able to write without causing delays to normal working.

[timvaillancourt]

What purpose we would solve to throttler on rdonly tablet types. primary tablets do not expect semi-sync acks from those tablets. We would be throttling user traffic when they can actually able to write without causing delays to normal working.

@harshit-gangal while I don't recommend it for most, in Slack's current use-case we read from rdonly tablets in some situations and "care" about the staleness of those reads from an application consistency perspective (vs durability)

[harshit-gangal]

What purpose we would solve to throttler on rdonly tablet types. primary tablets do not expect semi-sync acks from those tablets. We would be throttling user traffic when they can actually able to write without causing delays to normal working.

@harshit-gangal while I don't recommend it for most, in Slack's current use-case we read from rdonly tablets in some situations and "care" about the staleness of those reads from an application consistency perspective (vs durability)

have you explored the flag discovery_high_replication_lag_minimum_serving? The purpose is to remove unhealthy tablets from serving traffic if they exceed the lag tolerance irrespective of not meeting the minimum tablets serving criteria.

[timvaillancourt]

have you explored the flag discovery_high_replication_lag_minimum_serving? The purpose is to remove unhealthy tablets from serving traffic if they exceed the lag tolerance irrespective of not meeting the minimum tablets serving criteria.

@harshit-gangal yes, we're using this to remove unhealthy replicas in prod, however losing availability to replicas causes other undesireable problems

The goal we have with including RDONLY is to reduce the rate of writes (tell low-priority clients to backoff) when we are exceeding the rate tablets can replicate to avoid both lag and loss of availability of these tablet types

cc @ejortegau / @shlomi-noach for more thoughts

[shlomi-noach]

The discussion took many turns by now, but I wish to comment about the use of RDONLY and semi-sync etc. I think this flag is in line with existing tablet throttler flag --throttle_tablet_types.
It's also worth pointing out that semi-sync has no strict dependency on lag. It is possible for all semi-sync replicas to ACK all binlog events, and also have minutes of lag. So on that respect, too, there is no problem imO with including RDONLY tablets.