Skip to content

[DRAFT] tablet picker refactor#12094

Closed
pbibra wants to merge 7 commits into
vitessio:mainfrom
slackhq:pbibra-tablet-picker-refactor
Closed

[DRAFT] tablet picker refactor#12094
pbibra wants to merge 7 commits into
vitessio:mainfrom
slackhq:pbibra-tablet-picker-refactor

Conversation

@pbibra

@pbibra pbibra commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Description

This PR adds options to the TabletPicker that allow for selecting cell preference in addition to changing default behavior to give priority to the local cell and any alias it belongs to. We are also introducing a new way to select tablet order which should eventually replace the in_order: hint currently included tabletTypesStr.

The following changes are made to the NewTabletPicker function signature (i.e. the TabletPicker's instantiation):

before:

func NewTabletPicker(ts *topo.Server, cells []string, keyspace, shard, tabletTypesStr string) (*TabletPicker, error) {

after:

func NewTabletPicker(
	ctx context.Context,
	ts *topo.Server,
	cells []string,
	localCell, keyspace, shard, tabletTypesStr string,
	options TabletPickerOptions,
) (*TabletPicker, error) {

Where ctx, localCell, option are all new parameters.

option is of type TabletPickerOptions and includes two fields, CellPref and TabletOrder.
CellPref -> "PreferLocalWithAlias" (default) or "OnlySpecified"
TabletOrder -> "Any" (default) or "InOrder"
The above are defined as enum strings.

The following illustrates the behavior with different option combinations. Assume the following setup:

localCell = us-east-1a
alias = region1 -> {us-east-1a, us-east-1b, us-east-1c}
`cells = {us-west-1a, us-west-1b}

tablets:

tablet-1-PRIMARY in us-east-1a
tablet-2-REPLICA in us-east-1a
tablet-3-PRIMARY in us-east-1b
tablet-4-REPLICA in us-east-1b
tablet-5-PRIMARY in us-west-1a
tablet-6-PRIMARY in us-west-1b
tablet-7-REPLICA in us-west-1b

When "PreferLocalWithAlias" is selected, localCell must be non-empty:

  1. With "Any" as the tablet order (or for backward compatibility the in order hint does not exist in the tabletTypeStr), the TabletPicker will first pick tablets in the localCell and subsequently pick tablets in any alias the local cell belongs to. Finally, it will pick tablets belonging to cells if any additional ones are specified. The tablet type in this scenario does not matter since "Any" was selected.

tabletTypeStr = REPLICA,PRIMARY
tablet priority possibility:

tablet-1-PRIMARY in us-east-1a
tablet-2-REPLICA in us-east-1a
tablet-3-PRIMARY in us-east-1b
tablet-4-REPLICA in us-east-1b
tablet-5-PRIMARY in us-west-1a
tablet-6-PRIMARY in us-west-1b
tablet-7-REPLICA in us-west-1b
  1. With "InOrder" as the tablet order (or for backward compatibility the in order hint exists in the tabletTypeStr), the TabletPicker will first pick tablets in the localCell that also respect the tablet type ordering and subsequently pick tablets in any alias the local cell belongs to with the tablet type order. Finally, it will pick tablets belonging to cells if any additional ones are specified. The tablet type in this scenario is respected within each grouping of tablets (i.e. local cell, alias, all others)

tabletTypeStr = in_order:REPLICA,PRIMARY
tablet priority possibility:

tablet-2-REPLICA in us-east-1a
tablet-1-PRIMARY in us-east-1a
tablet-4-REPLICA in us-east-1b
tablet-3-PRIMARY in us-east-1b
tablet-5-PRIMARY in us-west-1a 
tablet-7-REPLICA in us-west-1b
tablet-6-PRIMARY in us-west-1b

When "OnlySpecified" is selected as the cell preference:

  1. With "Any" as the tablet order (or for backward compatibility the in order hint does not exist in the tabletTypeStr), the TabletPicker will only pick tablets belonging to cells specified in the cells parameter. It will ignore tablet type ordering in this case.

tabletTypeStr = REPLICA,PRIMARY
tablet priority possibility:

tablet-6-PRIMARY in us-west-1b
tablet-7-REPLICA in us-west-1b
tablet-5-PRIMARY in us-west-1a 
  1. With "InOrder" as the tablet order (or for backward compatibility the in order hint exists in the tabletTypeStr), the TabletPicker will only pick tablets belonging to cells specified in the cells parameter, but will respect the tablet type ordering.

tabletTypeStr = in_order:REPLICA,PRIMARY
tablet priority possibility:

tablet-7-REPLICA in us-west-1b
tablet-6-PRIMARY in us-west-1b
tablet-5-PRIMARY in us-west-1a 

Related Issue(s)

#11999

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

@vitess-bot vitess-bot Bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Jan 13, 2023
@vitess-bot

vitess-bot Bot commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

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.

@mattlord mattlord self-assigned this Jan 13, 2023
Comment thread go/vt/discovery/tablet_picker.go Outdated
// All provided tablet types are given equal priority. This is the default.
TabletPickerTabletOrder_Any TabletPickerTabletOrder = "Any"
// Provided tablet types are expected to be prioritized in the given order.
TabletPickerTabletOrder_InOrder TabletPickerTabletOrder = "InOrder"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

change these to iota

Comment thread go/vt/discovery/tablet_picker.go Outdated
ts *topo.Server,
cells []string,
localCell, keyspace, shard, tabletTypesStr string,
options *TabletPickerOptions,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pass by copy

Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com>
Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com>
Signed-off-by: Priya Bibra <pbibra@slack-corp.com>
@pbibra

pbibra commented Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

closing in favor of: #12282

@pbibra pbibra closed this Feb 8, 2023
@pbibra pbibra deleted the pbibra-tablet-picker-refactor branch February 8, 2023 21:05
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 NeedsWebsiteDocsUpdate What it says

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants