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

slack-vitess-r14.0.5-dsdefense: Add flag to select tx throttler tablet type (vitessio#12174) #95

Conversation

timvaillancourt
Copy link
Member

@timvaillancourt timvaillancourt commented Jun 28, 2023

Description

This backports the upstream PR vitessio#12174

A few tweaks needed to be made in this backport because topoproto.TabletTypeListFlag doesn't exist in v14, because flag parsing hadn't totally moved to github.com/spf13/pflag. Specifically, tweaks had to be made to set the flag default in .Init(), which caused some unit tests to fail until they were tweaked as well. Also a "rice box" had to be updated - something that no longer exists on upstream main

Related Issue(s)

vitessio#12174

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

Deployment Notes

@timvaillancourt timvaillancourt requested a review from a team as a code owner June 28, 2023 15:03
@timvaillancourt timvaillancourt changed the title Add flag to select tx throttler tablet type (vitessio#12174) slack-vitess-r14.0.5-dsdefense: Add flag to select tx throttler tablet type (vitessio#12174) Jun 28, 2023
@timvaillancourt timvaillancourt marked this pull request as draft June 29, 2023 09:00
@timvaillancourt timvaillancourt force-pushed the pr-12174-slack-vitess-r14.0.5-dsdefense branch from 8382f2a to 3642d12 Compare June 30, 2023 16:42
@timvaillancourt timvaillancourt marked this pull request as ready for review June 30, 2023 16:43
@timvaillancourt timvaillancourt force-pushed the pr-12174-slack-vitess-r14.0.5-dsdefense branch from 960097c to b8d1ff9 Compare June 30, 2023 17:29
@timvaillancourt timvaillancourt force-pushed the pr-12174-slack-vitess-r14.0.5-dsdefense branch from 9162ea9 to 1cee40c Compare June 30, 2023 21:18
Signed-off-by: Tim Vaillancourt <[email protected]>
case topodatapb.TabletType_REPLICA, topodatapb.TabletType_RDONLY:
continue
default:
return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported tablet type %q", tabletType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making the error message be more explicit about the problem being with the tablet type for the tx throttler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ejortegau good point 👍. This matches upstream so I'd prefer to keep this PR as-is, but I'll fix this in an upcoming (hopefully final) cleanup PR

Would txThrottler: tablet type %q is not one of supported types replica and rdonly work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@timvaillancourt timvaillancourt merged commit d7a37b3 into slack-vitess-r14.0.5-dsdefense Jul 5, 2023
@timvaillancourt timvaillancourt deleted the pr-12174-slack-vitess-r14.0.5-dsdefense branch July 5, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants