-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
tablet picker cell alias fallback with local cell preference #11771
Changes from all commits
6bd1fa8
34b1f1e
b96b709
53ac9fd
a16ee7d
f8d5f11
48dc388
b4f37ce
0dd9359
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,12 @@ package discovery | |
|
||
import ( | ||
"context" | ||
"os" | ||
"testing" | ||
"time" | ||
|
||
_flag "vitess.io/vitess/go/internal/flag" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"google.golang.org/protobuf/proto" | ||
|
@@ -364,6 +367,156 @@ func TestPickUsingCellAlias(t *testing.T) { | |
assert.True(t, picked2) | ||
} | ||
|
||
func TestPickLocalPreferences(t *testing.T) { | ||
type tablet struct { | ||
id uint32 | ||
typ topodatapb.TabletType | ||
cell string | ||
} | ||
|
||
type testCase struct { | ||
name string | ||
|
||
//inputs | ||
tablets []tablet | ||
inCells []string | ||
inTabletTypes string | ||
|
||
//expected | ||
tpLocalPreference string | ||
tpCells []string | ||
wantTablets []uint32 | ||
} | ||
|
||
tcases := []testCase{ | ||
{ | ||
"local preference", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this would be much easier to read if we had the struct field names, e.g.:
Otherwise the reader needs to keep the struct's field indexes in their head. |
||
[]tablet{ | ||
{101, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell2"}, | ||
{103, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"local:cell2", "cell1"}, | ||
"replica", | ||
"cell2", | ||
[]string{"cell1", "cell2"}, | ||
[]uint32{102, 103}, | ||
}, | ||
{ | ||
"local preference with cell alias", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"local:cell2", "cella"}, | ||
"replica", | ||
"cell2", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{102}, | ||
}, | ||
{ | ||
"local preference with tablet type ordering, replica", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{103, topodatapb.TabletType_PRIMARY, "cell2"}, | ||
{104, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"local:cell2", "cella"}, | ||
"in_order:replica,primary", | ||
"cell2", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{104}, | ||
}, | ||
{ | ||
"no local preference with tablet type ordering, primary", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{102, topodatapb.TabletType_PRIMARY, "cell1"}, | ||
{103, topodatapb.TabletType_REPLICA, "cell2"}, | ||
{104, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"cell2", "cella"}, | ||
"in_order:primary,replica", | ||
"", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{102}, | ||
}, | ||
{ | ||
"local preference with tablet type ordering, primary in local", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{103, topodatapb.TabletType_PRIMARY, "cell2"}, | ||
{104, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"local:cell2", "cella"}, | ||
"in_order:primary,replica", | ||
"cell2", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{103}, | ||
}, | ||
{ | ||
"local preference with tablet type ordering, primary not local", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_PRIMARY, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell1"}, | ||
{103, topodatapb.TabletType_REPLICA, "cell2"}, | ||
{104, topodatapb.TabletType_REPLICA, "cell2"}, | ||
}, | ||
[]string{"local:cell2", "cella"}, | ||
"in_order:primary,replica", | ||
"cell2", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{103, 104}, // replicas are picked because primary is not in the local cell/cell alias | ||
}, | ||
{ | ||
"local preference with tablet type ordering, primary in local's alias", | ||
[]tablet{ | ||
{101, topodatapb.TabletType_PRIMARY, "cell1"}, | ||
{102, topodatapb.TabletType_REPLICA, "cell1"}, | ||
}, | ||
[]string{"local:cell2", "cella"}, | ||
"in_order:primary,replica", | ||
"cell2", | ||
[]string{"cella", "cell2"}, | ||
[]uint32{101}, // primary found since there are no tablets in cell/cell alias | ||
}, | ||
} | ||
|
||
ctx := context.Background() | ||
for _, tcase := range tcases { | ||
t.Run(tcase.name, func(t *testing.T) { | ||
cells := []string{"cell1", "cell2"} | ||
te := newPickerTestEnv(t, cells) | ||
var testTablets []*topodatapb.Tablet | ||
for _, tab := range tcase.tablets { | ||
testTablets = append(testTablets, addTablet(te, int(tab.id), tab.typ, tab.cell, true, true)) | ||
} | ||
defer func() { | ||
for _, tab := range testTablets { | ||
deleteTablet(t, te, tab) | ||
} | ||
}() | ||
tp, err := NewTabletPicker(te.topoServ, tcase.inCells, te.keyspace, te.shard, tcase.inTabletTypes) | ||
require.NoError(t, err) | ||
require.Equal(t, tp.localPreference, tcase.tpLocalPreference) | ||
require.ElementsMatch(t, tp.cells, tcase.tpCells) | ||
var selectedTablets []uint32 | ||
selectedTabletMap := make(map[uint32]bool) | ||
for i := 0; i < 20; i++ { | ||
tab, err := tp.PickForStreaming(ctx) | ||
require.NoError(t, err) | ||
selectedTabletMap[tab.Alias.Uid] = true | ||
} | ||
for uid := range selectedTabletMap { | ||
selectedTablets = append(selectedTablets, uid) | ||
} | ||
require.ElementsMatch(t, selectedTablets, tcase.wantTablets) | ||
}) | ||
} | ||
} | ||
|
||
func TestTabletAppearsDuringSleep(t *testing.T) { | ||
te := newPickerTestEnv(t, []string{"cell"}) | ||
tp, err := NewTabletPicker(te.topoServ, te.cells, te.keyspace, te.shard, "replica") | ||
|
@@ -428,6 +581,11 @@ type pickerTestEnv struct { | |
topoServ *topo.Server | ||
} | ||
|
||
func TestMain(m *testing.M) { | ||
_flag.ParseFlagsForTest() | ||
os.Exit(m.Run()) | ||
} | ||
|
||
func newPickerTestEnv(t *testing.T, cells []string) *pickerTestEnv { | ||
ctx := context.Background() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,7 +161,7 @@ func (tw *TopologyWatcher) loadTablets() { | |
return | ||
default: | ||
} | ||
log.Errorf("cannot get tablets for cell: %v: %v", tw.cell, err) | ||
log.Errorf("cannot get tablets for cell:%v: %v", tw.cell, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this would be slightly better:
|
||
return | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested tweaks:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that we have in-progress VStream gRPC docs in: vitessio/website#1216
Preview: https://deploy-preview-1216--vitess.netlify.app/docs/16.0/reference/vreplication/vstream/