Skip to content

Commit

Permalink
[release-16.0] VDiff: correct handling of default source and target c…
Browse files Browse the repository at this point in the history 
…ells (#13969) (#13983)

Signed-off-by: Matt Lord <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Matt Lord <[email protected]>
  • Loading branch information
@vitess-bot @mattlord
vitess-bot[bot] and mattlord authored Sep 14, 2023
1 parent d75b9dc commit 09b2927
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 9 deletions.
48 changes: 43 additions & 5 deletions go/vt/discovery/tablet_picker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ import (
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"

querypb "vitess.io/vitess/go/vt/proto/query"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/topo/memorytopo"

querypb "vitess.io/vitess/go/vt/proto/query"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
)

func TestPickSimple(t *testing.T) {
Expand Down Expand Up @@ -364,6 +365,38 @@ func TestPickUsingCellAlias(t *testing.T) {
assert.True(t, picked2)
}

// TestPickUsingCellAsAlias confirms that when the tablet picker is
// given a cell name that is an alias, it will choose a tablet that
// exists within a cell that is part of the alias.
func TestPickUsingCellAsAlias(t *testing.T) {
ctx := context.Background()

// The test env puts all cells into an alias called "cella".
// We're also going to specify an optional extraCell that is NOT
// added to the alias.
te := newPickerTestEnv(t, []string{"cell1", "cell2", "cell3"}, "xtracell")
// Specify the alias as the cell.
tp, err := NewTabletPicker(te.topoServ, []string{"cella"}, te.keyspace, te.shard, "replica")
require.NoError(t, err)

// Create a tablet in one of the main cells, it should be
// picked as it is part of the cella alias. This tablet is
// NOT part of the talbet picker's local cell (cell1) so it
// will not be given local preference.
want := addTablet(te, 101, topodatapb.TabletType_REPLICA, "cell2", true, true)
defer deleteTablet(t, te, want)
// Create a tablet in an extra cell which is thus NOT part of
// the cella alias so it should NOT be picked.
noWant := addTablet(te, 102, topodatapb.TabletType_REPLICA, "xtracell", true, true)
defer deleteTablet(t, te, noWant)
// Try it many times to be sure we don't ever pick the wrong one.
for i := 0; i < 100; i++ {
tablet, err := tp.PickForStreaming(ctx)
require.NoError(t, err)
assert.True(t, proto.Equal(want, tablet), "Pick: %v, want %v", tablet, want)
}
}

func TestTabletAppearsDuringSleep(t *testing.T) {
te := newPickerTestEnv(t, []string{"cell"})
tp, err := NewTabletPicker(te.topoServ, te.cells, te.keyspace, te.shard, "replica")
Expand Down Expand Up @@ -428,17 +461,22 @@ type pickerTestEnv struct {
topoServ *topo.Server
}

func newPickerTestEnv(t *testing.T, cells []string) *pickerTestEnv {
// newPickerTestEnv creates a test environment for TabletPicker tests.
// It creates a cell alias called 'cella' which contains all of the
// provided cells. However, if any optional extraCells are provided, those
// are NOT added to the cell alias.
func newPickerTestEnv(t *testing.T, cells []string, extraCells ...string) *pickerTestEnv {
ctx := context.Background()
allCells := append(cells, extraCells...)

te := &pickerTestEnv{
t: t,
keyspace: "ks",
shard: "0",
cells: cells,
topoServ: memorytopo.NewServer(cells...),
topoServ: memorytopo.NewServer(allCells...),
}
// create cell alias
// Create cell alias containing the cells (but NOT the extraCells).
err := te.topoServ.CreateCellsAlias(ctx, "cella", &topodatapb.CellsAlias{
Cells: cells,
})
Expand Down
14 changes: 11 additions & 3 deletions go/vt/vttablet/tabletmanager/vdiff/action.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"context"
"encoding/json"
"fmt"
"sort"
"strings"

"github.com/google/uuid"

Expand Down Expand Up @@ -108,6 +110,9 @@ func (vde *Engine) getVDiffSummary(vdiffID int64, dbClient binlogplayer.DBClient
// Validate vdiff options. Also setup defaults where applicable.
func (vde *Engine) fixupOptions(options *tabletmanagerdatapb.VDiffOptions) (*tabletmanagerdatapb.VDiffOptions, error) {
// Assign defaults to sourceCell and targetCell if not specified.
if options == nil {
options = &tabletmanagerdatapb.VDiffOptions{}
}
sourceCell := options.PickerOptions.SourceCell
targetCell := options.PickerOptions.TargetCell
var defaultCell string
Expand All @@ -118,10 +123,10 @@ func (vde *Engine) fixupOptions(options *tabletmanagerdatapb.VDiffOptions) (*tab
return nil, err
}
}
if sourceCell == "" {
if sourceCell == "" { // Default is all cells
sourceCell = defaultCell
}
if targetCell == "" {
if targetCell == "" { // Default is all cells
targetCell = defaultCell
}
options.PickerOptions.SourceCell = sourceCell
Expand All @@ -130,6 +135,8 @@ func (vde *Engine) fixupOptions(options *tabletmanagerdatapb.VDiffOptions) (*tab
return options, nil
}

// getDefaultCell returns all of the cells in the topo as a comma
// separated string as the default value is all available cells.
func (vde *Engine) getDefaultCell() (string, error) {
cells, err := vde.ts.GetCellInfoNames(vde.ctx)
if err != nil {
Expand All @@ -139,7 +146,8 @@ func (vde *Engine) getDefaultCell() (string, error) {
// Unreachable
return "", fmt.Errorf("there are no cells in the topo")
}
return cells[0], nil
sort.Strings(cells) // Ensure that the resulting value is deterministic
return strings.Join(cells, ","), nil
}

func (vde *Engine) handleCreateResumeAction(ctx context.Context, dbClient binlogplayer.DBClient, action VDiffAction, req *tabletmanagerdatapb.VDiffRequest, resp *tabletmanagerdatapb.VDiffResponse) error {
Expand Down
71 changes: 70 additions & 1 deletion go/vt/vttablet/tabletmanager/vdiff/action_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,14 @@ import (
"time"

"github.com/google/uuid"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/vterrors"

tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
topodatapb "vitess.io/vitess/go/vt/proto/topodata"
vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
"vitess.io/vitess/go/vt/vterrors"
)

func TestPerformVDiffAction(t *testing.T) {
Expand All @@ -43,6 +46,8 @@ func TestPerformVDiffAction(t *testing.T) {
name string
vde *Engine
req *tabletmanagerdatapb.VDiffRequest
preFunc func() error
postFunc func() error
want *tabletmanagerdatapb.VDiffResponse
expectQueries []string
wantErr error
Expand All @@ -52,6 +57,62 @@ func TestPerformVDiffAction(t *testing.T) {
vde: &Engine{isOpen: false},
wantErr: vterrors.New(vtrpcpb.Code_UNAVAILABLE, "vdiff engine is closed"),
},
{
name: "create with defaults",
req: &tabletmanagerdatapb.VDiffRequest{
Action: string(CreateAction),
VdiffUuid: uuid,
Options: &tabletmanagerdatapb.VDiffOptions{
PickerOptions: &tabletmanagerdatapb.VDiffPickerOptions{},
},
},
// Add a second cell. The default for source_cell and target_cell is all
// available cells, so this additional cell should then show up in the
// created vdiff record.
preFunc: func() error {
return tstenv.TopoServ.CreateCellInfo(ctx, "zone100_test", &topodatapb.CellInfo{})
},
expectQueries: []string{
fmt.Sprintf("select id as id from _vt.vdiff where vdiff_uuid = %s", encodeString(uuid)),
fmt.Sprintf(`insert into _vt.vdiff(keyspace, workflow, state, options, shard, db_name, vdiff_uuid) values('', '', 'pending', '{\"picker_options\":{\"source_cell\":\"cell1,zone100_test\",\"target_cell\":\"cell1,zone100_test\"}}', '0', 'vt_vttest', %s)`, encodeString(uuid)),
},
postFunc: func() error {
return tstenv.TopoServ.DeleteCellInfo(ctx, "zone100_test", true)
},
},
{
name: "create with cell alias",
req: &tabletmanagerdatapb.VDiffRequest{
Action: string(CreateAction),
VdiffUuid: uuid,
Options: &tabletmanagerdatapb.VDiffOptions{
PickerOptions: &tabletmanagerdatapb.VDiffPickerOptions{
SourceCell: "all",
TargetCell: "all",
},
},
},
// Add a second cell and create an cell alias that contains it.
preFunc: func() error {
if err := tstenv.TopoServ.CreateCellInfo(ctx, "zone100_test", &topodatapb.CellInfo{}); err != nil {
return err
}
cells := append(tstenv.Cells, "zone100_test")
return tstenv.TopoServ.CreateCellsAlias(ctx, "all", &topodatapb.CellsAlias{
Cells: cells,
})
},
expectQueries: []string{
fmt.Sprintf("select id as id from _vt.vdiff where vdiff_uuid = %s", encodeString(uuid)),
fmt.Sprintf(`insert into _vt.vdiff(keyspace, workflow, state, options, shard, db_name, vdiff_uuid) values('', '', 'pending', '{\"picker_options\":{\"source_cell\":\"all\",\"target_cell\":\"all\"}}', '0', 'vt_vttest', %s)`, encodeString(uuid)),
},
postFunc: func() error {
if err := tstenv.TopoServ.DeleteCellInfo(ctx, "zone100_test", true); err != nil {
return err
}
return tstenv.TopoServ.DeleteCellsAlias(ctx, "all")
},
},
{
name: "delete by uuid",
req: &tabletmanagerdatapb.VDiffRequest{
Expand Down Expand Up @@ -80,6 +141,10 @@ func TestPerformVDiffAction(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.preFunc != nil {
err := tt.preFunc()
require.NoError(t, err, "pre function failed: %v", err)
}
if tt.vde == nil {
tt.vde = vdiffenv.vde
}
Expand All @@ -94,6 +159,10 @@ func TestPerformVDiffAction(t *testing.T) {
if tt.want != nil && !reflect.DeepEqual(got, tt.want) {
t.Errorf("Engine.PerformVDiffAction() = %v, want %v", got, tt.want)
}
if tt.postFunc != nil {
err := tt.postFunc()
require.NoError(t, err, "post function failed: %v", err)
}
})
}
}

0 comments on commit 09b2927

Please sign in to comment.