Prefer using REPLICA tablets when selecting vreplication sources #10040
Conversation
Signed-off-by: Matt Lord <[email protected]>
mattlord
requested review from
rohit-nayak-ps,
ajm188,
doeg and
notfelineit
as code owners
mattlord
added
Component: VReplication
Type: Enhancement
Signed-off-by: Matt Lord <[email protected]>
go/vt/vtctl/vtctl.go
Outdated
| @@ -2330,7 +2330,7 @@ func commandVRWorkflow(ctx context.Context, wr *wrangler.Wrangler, subFlags *fla | |||
| const defaultMaxReplicationLagAllowed = defaultWaitTime | |||
|
|
|||
| cells := subFlags.String("cells", "", "Cell(s) or CellAlias(es) (comma-separated) to replicate from.") | |||
| tabletTypes := subFlags.String("tablet_types", "primary,replica,rdonly", "Source tablet types to replicate from (e.g. primary, replica, rdonly). Defaults to -vreplication_tablet_type parameter value for the tablet, which has the default value of replica.") | |||
| tabletTypes := subFlags.String("tablet_types", "in_order:REPLICA,PRIMARY", "Source tablet types to replicate from (e.g. primary, replica, rdonly). Defaults to --vreplication_tablet_type parameter value for the tablet, which has the default value of in_order:REPLICA,PRIMARY.") | |||
There was a problem hiding this comment.
nit: we seem to be using uppercase in the default value, but lowercase in the help text. It doesn't matter for the internals, but it will be good to make this consistent everywhere in the user interface (including the vreplication_tablet_type flag to vttablet).
There was a problem hiding this comment.
Where do you see it lower case?
There was a problem hiding this comment.
ah, you mean more generally than the default value itself e.g. (e.g. primary, replica, rdonly). I prefer capitalized but don't feel too strongly about it.
There was a problem hiding this comment.
At least started doing that here ebc0469 and here vitessio/website@679678e
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
| @@ -2486,7 +2486,7 @@ func commandVRWorkflow(ctx context.Context, wr *wrangler.Wrangler, subFlags *fla | |||
| vrwp.Cells = *cells | |||
| vrwp.TabletTypes = *tabletTypes | |||
| if vrwp.TabletTypes == "" { | |||
| vrwp.TabletTypes = "primary,replica,rdonly" | |||
| vrwp.TabletTypes = "in_order:REPLICA,PRIMARY" | |||
There was a problem hiding this comment.
Was rdonly removed on purpose?
There was a problem hiding this comment.
Yeah, according to our docs and --help output we use the default of the vttablet --vreplication_tablet_type flag if you don't specify and that's what we're actually now doing here. After this PR, the only place RDONLY is potentially used by default is for VDiff (maybe rdonly had ended up here due to a cut and paste from the VDiff code?).
Let me know if not using the vttablet --vreplication_tablet_type default was intentional here before and perhaps the docs were incorrect.
There was a problem hiding this comment.
Related to this, I'm not sure why we have this if vrwp.TabletTypes == "" { code block at all since we are specifying a default value here: https://github.com/vitessio/vitess/pull/10040/files#diff-ad37010accc5a29c6751d18124328bb882457e8b2527db688b854c81fe23861cR2333
Maybe that wasn't always working as I'd expect.
There was a problem hiding this comment.
Related to this, I'm not sure why we have this
if vrwp.TabletTypes == "" {code block at all since we are specifying a
This might be defensive coding for when an empty string is specified for tablet types on the command line or in tests.
There was a problem hiding this comment.
@rohit-nayak-ps turns out that you were right! I shouldn't have removed that. Re-adding it fix this: #10421
🤦
There was a problem hiding this comment.
Well, after further testing... this didn't actually change any behavior. We were never switching RDONLY traffic by default -- as the tablet_types flag variable was never an empty string as it has a default value (which is what we changed) -- and we weren't testing for this either. Will correct both in #10421.
…essio#10040) * Prefer using REPLICA tablets when selecting vreplication sources Signed-off-by: Matt Lord <[email protected]> * Modify tablet type defaults for generic vrepl workflow handler Signed-off-by: Matt Lord <[email protected]> * Add in_order support to the v2 workflow code Signed-off-by: Matt Lord <[email protected]> * Minor changes after self review Signed-off-by: Matt Lord <[email protected]> * Correct primary tablet type filtering in switchReads Signed-off-by: Matt Lord <[email protected]> * Consistently use capitalization for tablet types Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Vilius Okockis <[email protected]>
Description
This changes the defaults for tablet selection when choosing vreplication sources so that we use a
REPLICAtablet when one is available and only fall back to using aPRIMARYtablet when necessary.This is a good default as the vreplication workflow can have a very significant impact on the source tablet and should generally not be done on a
PRIMARYtablet when possible since it's likely serving production traffic.Related Issue(s)
Checklist