Update tabletpicker to support cell pref and tablet order options by pbibra · Pull Request #12282 · vitessio/vitess

pbibra · 2023-02-08T21:01:15Z

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:

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

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:

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

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)

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
docs for tabletpicker refactor website#1441

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

vitess-bot · 2023-02-08T21:01:18Z

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

github-actions · 2023-03-11T01:32:26Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

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>

mattlord

Overall it looks good! Mostly minor comments/suggestions.

Beyond that, I don't see where we've added any new unit or e2e tests for this new behavior (exists tests are using the equivalent of the old behavior). We should add unit tests for this new behavior and modify the e2e tests as well, at least to use the new default preference.

pbibra · 2023-04-12T23:03:24Z

Overall it looks good! Mostly minor comments/suggestions.

Beyond that, I don't see where we've added any new unit or e2e tests for this new behavior (exists tests are using the equivalent of the old behavior). We should add unit tests for this new behavior and modify the e2e tests as well, at least to use the new default preference.

I did add some new unit tests in tablet_picker_test.go. Let me know if those look good! Good call out on the e2e tests though, I can look at adding some new ones there as well.

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>

mattlord

This is looking great, @pbibra! I have some minor comments that I think we can address pretty quickly. Please let m know if any of them are unclear. ❤️

mattlord · 2023-04-27T18:17:26Z


 - **[Major Changes](#major-changes)**
  - **[Breaking Changes](#breaking-changes)**
+    - [Default Local Cell Preference for TabletPicker](#tablet-picker-cell-preference)


I don't think this needs to be in breaking changes. But I guess it doesn't hurt to increase awareness just in case we're missing some breaking case.

I figured since we're changing default behavior it's good to advertise that? Let me know if there's another better place to do that

mattlord · 2023-04-27T19:02:54Z

 	_, err = tp.PickForStreaming(ctx)
 	require.EqualError(t, err, "context has expired")
+	// if local preference is selected, tp cells include's the local cell's alias
+	require.Greater(t, globalTPStats.noTabletFoundError.Counts()["cell_cella.ks.0.replica"], int64(0))


And we assume in this case that we then use the alias?

mattlord · 2023-04-27T19:05:15Z

+	tp, err := NewTabletPicker(context.Background(), te.topoServ, te.cells, "cell", te.keyspace, te.shard, "replica", TabletPickerOptions{CellPreference: "OnlySpecified"})
+	require.NoError(t, err)
+	delay := GetTabletPickerRetryDelay()
+	defer func() {
+		SetTabletPickerRetryDelay(delay)
+	}()
+	SetTabletPickerRetryDelay(11 * time.Millisecond)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond)
+	defer cancel()
+	// no tablets
+	_, err = tp.PickForStreaming(ctx)
+	require.EqualError(t, err, "context has expired")
+	// no tablets of the correct type
+	defer deleteTablet(t, te, addTablet(te, 200, topodatapb.TabletType_RDONLY, "cell", true, true))
+	ctx, cancel = context.WithTimeout(context.Background(), 20*time.Millisecond)
+	defer cancel()
+	_, err = tp.PickForStreaming(ctx)
+	require.EqualError(t, err, "context has expired")


It's not obvious to me how we're testing OnlySpecified here? Am I missing something?

this is showing that if the OnlySpecified option is passed into the tablet picker, but no tablet exists of that specified type, then picker cannot select a tablet

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

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

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>

mattlord

LGTM! Thank you for all of the time you put into this, @pbibra !

rohit-nayak-ps

Nice work! Thanks for this very useful contribution.

…tessio#12282) * update tabletpicker to support cell pref and tablet order options Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add tablet picker options in vstream flags Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add tablet picker tests Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add summary docs Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * update proto Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix vreplication test Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix doc typos, add function comments, update error handling Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix tests and comments Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * fix local cell ref Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * update proto Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add log line to debug test Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add cell to vre test def Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * define vrepl engine for controller tests 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> Signed-off-by: pbibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> Signed-off-by: Vilius Okockis <vilius.okockis@vinted.com>

…tessio#12282) * update tabletpicker to support cell pref and tablet order options Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add tablet picker options in vstream flags Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add tablet picker tests Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add summary docs Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * update proto Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix vreplication test Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix doc typos, add function comments, update error handling Signed-off-by: Priya Bibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com> * fix tests and comments Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * fix local cell ref Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * update proto Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add log line to debug test Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * add cell to vre test def Signed-off-by: Priya Bibra <pbibra@slack-corp.com> * define vrepl engine for controller tests 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> Signed-off-by: pbibra <pbibra@slack-corp.com> Signed-off-by: 'Priya Bibra' <pbibra@slack-corp.com>

pbibra added 3 commits February 8, 2023 12:34

update tabletpicker to support cell pref and tablet order options

c1f1ec8

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

add tablet picker options in vstream flags

00a42a0

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

add tablet picker tests

d0aa247

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

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

Merge branch 'main' into pbibra-refactor-tabletpicker

799c5a6

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

pbibra mentioned this pull request Feb 8, 2023

[DRAFT] tablet picker refactor #12094

Closed

3 tasks

rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication Component: TabletManager and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work labels Feb 8, 2023

github-actions Bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Mar 11, 2023

mattlord removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Mar 17, 2023

mattlord self-assigned this Mar 17, 2023

pbibra force-pushed the pbibra-refactor-tabletpicker branch 3 times, most recently from 278eabd to 799c5a6 Compare March 30, 2023 00:47

pbibra added 4 commits March 29, 2023 17:49

Merge branch 'main' into pbibra-refactor-tabletpicker

643e8f5

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

add summary docs

7402773

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

update proto

ebbcfde

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

fix vreplication test

8697653

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

pbibra mentioned this pull request Apr 10, 2023

docs for tabletpicker refactor vitessio/website#1441

Merged

mattlord removed the NeedsWebsiteDocsUpdate What it says label Apr 10, 2023

pbibra marked this pull request as ready for review April 10, 2023 18:54

pbibra requested review from deepthi, frouioui, harshit-gangal and rohit-nayak-ps as code owners April 10, 2023 18:54

pbibra requested review from rsajwani and shlomi-noach as code owners April 10, 2023 18:54

mattlord changed the title ~~[DRAFT] update tabletpicker to support cell pref and tablet order options~~ Update tabletpicker to support cell pref and tablet order options Apr 10, 2023

mattlord reviewed Apr 11, 2023

View reviewed changes

pbibra added 2 commits April 12, 2023 16:06

Merge branch 'main' into pbibra-refactor-tabletpicker

910d921

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

fix doc typos, add function comments, update error handling

0553f40

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

mattlord self-requested a review April 26, 2023 18:45

mattlord reviewed Apr 27, 2023

View reviewed changes

pbibra added 4 commits April 30, 2023 18:43

fix tests and comments

769c608

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

Merge branch 'main' into pbibra-refactor-tabletpicker

acb23ca

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

fix local cell ref

acdf84c

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

update proto

466a606

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

mattlord removed the request for review from rsajwani May 1, 2023 15:01

pbibra added 3 commits May 1, 2023 16:32

add log line to debug test

b5ff9c4

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

add cell to vre test def

638fa63

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

define vrepl engine for controller tests

740d791

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

mattlord self-requested a review May 8, 2023 14:58

mattlord approved these changes May 9, 2023

View reviewed changes

rohit-nayak-ps approved these changes May 18, 2023

View reviewed changes

rohit-nayak-ps merged commit 2dd3853 into vitessio:main May 18, 2023

pbibra mentioned this pull request Sep 25, 2023

revert custom local cell preference and apply upstream 12282 slackhq/vitess#135

Merged

timvaillancourt deleted the pbibra-refactor-tabletpicker branch May 29, 2024 00:23

timvaillancourt mentioned this pull request May 29, 2024

slack-15.0: VStreams fix backports/patches, pt. 2 slackhq/vitess#380

Merged

4 tasks

rohit-nayak-ps mentioned this pull request Jun 20, 2024

RFC: VTGate and Tablet Picker: prefer local cells in source tablet selection for vreplication workflows and the VStream API #11999

Closed

mattlord mentioned this pull request Aug 9, 2024

Retry VStream errors with exponential backoff and switch tablets for repeated errors #16536

Closed

5 tasks

rohit-nayak-ps mentioned this pull request Sep 29, 2024

Feature Request: Specifying multiple TabletTypes (e.g. REPLICA,PRIMARY) during VStream client subscription. #11579

Closed

Conversation

pbibra commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Uh oh!

vitess-bot Bot commented Feb 8, 2023

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

github-actions Bot commented Mar 11, 2023

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pbibra commented Apr 12, 2023

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

mattlord Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

pbibra May 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattlord Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

pbibra May 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattlord Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

pbibra May 1, 2023

Choose a reason for hiding this comment

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pbibra commented Feb 8, 2023 •

edited

Loading