Skip to content

Commit

Permalink
[DAP-5348] Retry instantiating Vindex if subvindex dependencies have …
Browse files Browse the repository at this point in the history 
…not yet been loaded on first attempt. (#25)

* [DAP-5348] Retry instantiating Vindex if subvindex dependencies have not yet been loaded on first attempt.

* Refactor BuildVindexes to return list of vindexes to retry.

* Return error in BuildVindexes if vindex depends on itself. Do not retry if subvindex does not exist in vschema. Add test cases for circular dependencies.
  • Loading branch information
@swu-etsy
swu-etsy authored Jul 1, 2024
1 parent a9fdeb4 commit cd271ef
Show file tree
Hide file tree
Showing 4 changed files with 369 additions and 55 deletions.
22 changes: 18 additions & 4 deletions go/vt/vtgate/vindexes/multisharded.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"encoding/json"
"fmt"
"strings"

"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/key"
Expand All @@ -32,7 +33,16 @@ func init() {
Register("etsy_multisharded_hybrid", NewMultiSharded)
}

// Multisharded defines a multicolumn vindex that resolves a provided
type MissingSubvindexError struct {
MissingSubvindexes []string
Method string
}

func (e *MissingSubvindexError) Error() string {
return fmt.Sprintf("%s: The following subvindexes have not been defined: %s", e.Method, strings.Join(e.MissingSubvindexes, ", "))
}

// MultiSharded defines a multicolumn vindex that resolves a provided
// typeId column value to a hybrid subvindex and applies the subvindex
// to the given id column value
type MultiSharded struct {
Expand All @@ -58,16 +68,20 @@ func NewMultiSharded(name string, m map[string]string) (Vindex, error) {
}

subvindexes := make(map[string]SingleColumn)
missingSubvindexes := []string{}
for _, vindexName := range typeIdToSubvindexName {
// Only hybrid vindexes defined above this etsy_multisharded_hybrid vindex in the vschema,
// and therefore initialized before this vschema, will be avaiable in `hybridVindexes`.
// Only hybrid vindexes instantiated before this vindex will be available in `hybridVindexes`.
if _, ok := hybridVindexes[vindexName]; ok {
subvindexes[vindexName] = hybridVindexes[vindexName]
} else {
return nil, fmt.Errorf("Multisharded.NewMultisharded: No hybrid vindex named %s has been defined", vindexName)
missingSubvindexes = append(missingSubvindexes, vindexName)
}
}

if len(missingSubvindexes) > 0 {
return nil, &MissingSubvindexError{MissingSubvindexes: missingSubvindexes, Method: "Multisharded.NewMultiSharded"}
}

return &MultiSharded{
name: name,
typeIdToSubvindexName: typeIdToSubvindexName,
Expand Down
79 changes: 42 additions & 37 deletions go/vt/vtgate/vindexes/multisharded_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,32 +81,58 @@ func TestMultiShardedCreation(t *testing.T) {

func TestMultiShardedCreationWithNonexistantSubvindex(t *testing.T) {
hybridVindexes = map[string]SingleColumn{
"etsy_hybrid_DNE": &HybridStub{},
"etsy_hybrid_user": &HybridStub{},
"etsy_hybrid_shop": &HybridStub{},
}

expectedMissingSubvindexes := map[string]bool{"etsy_hybrid_DNE": true, "etsy_hybrid_DNE_2": true}
expectedMissingSubvindexesSlice := []string{}
for expectedSubvindex := range expectedMissingSubvindexes {
expectedMissingSubvindexesSlice = append(expectedMissingSubvindexesSlice, expectedSubvindex)
}

params := map[string]string{
"type_id_to_vindex": `{"1":"etsy_hybrid_user", "2":"etsy_hybrid_shop"}`,
"type_id_to_vindex": `{"1":"etsy_hybrid_DNE", "2":"etsy_hybrid_shop", "3":"etsy_hybrid_DNE_2"}`,
}

expectedName := "multisharded_test"
_, err := CreateVindex("etsy_multisharded_hybrid", expectedName, params)

if err == nil {
t.Errorf("Expected error from multisharded.NewMultiSharded, got nil")
t.Errorf("Expected MissingSubvindexError from multisharded.NewMultiSharded, got nil")
}

if _, ok := err.(*MissingSubvindexError); !ok {
t.Errorf("Expected MissingSubvindexError from multisharded.NewMultiSharded, got %s", err.Error())
}

if len(expectedMissingSubvindexes) != len(err.(*MissingSubvindexError).MissingSubvindexes) {
t.Errorf(
"Got unexpected value for MissingSubvindexError.MissingSubvindexes. Expected: %v, Got: %v",
expectedMissingSubvindexesSlice,
err.(*MissingSubvindexError).MissingSubvindexes)
}

for _, subvindex := range err.(*MissingSubvindexError).MissingSubvindexes {
if _, ok := expectedMissingSubvindexes[subvindex]; !ok {
t.Errorf(
"Got unexpected value for MissingSubvindexError.MissingSubvindexes. Expected: %v, Got: %v",
expectedMissingSubvindexesSlice,
err.(*MissingSubvindexError).MissingSubvindexes)

}
}
}

func TestMultiShardedMap(t *testing.T) {
cases := []struct {
name string
typeIdToVindex string
rowsColValues [][]sqltypes.Value
expected []key.Destination
shouldErr bool
name string
rowsColValues [][]sqltypes.Value
expected []key.Destination
shouldErr bool
}{
{
"All rows map to same subvindex",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(5)},
Expand All @@ -121,7 +147,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Rows map to different subvindexes",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(2), sqltypes.NewInt64(60)},
Expand All @@ -136,7 +161,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Some rows map to subvindexes",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(2), sqltypes.NewInt64(60)},
Expand All @@ -147,7 +171,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"No rows map to subvindexes",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(100), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(200), sqltypes.NewInt64(60)},
Expand All @@ -158,7 +181,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Errors when type_id is negative int",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(-1), sqltypes.NewInt64(100)},
},
Expand All @@ -167,7 +189,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Errors when id is negative int",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(-100)},
},
Expand All @@ -176,7 +197,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Errors when type_id is negative string",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("-1"), sqltypes.NewVarChar("100")},
},
Expand All @@ -185,7 +205,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"Errors when id is negative string",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("1"), sqltypes.NewVarChar("-100")},
},
Expand All @@ -194,7 +213,6 @@ func TestMultiShardedMap(t *testing.T) {
},
{
"String ids supported",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("1"), sqltypes.NewVarChar("1")},
{sqltypes.NewVarBinary("2"), sqltypes.NewVarBinary("60")},
Expand All @@ -214,7 +232,7 @@ func TestMultiShardedMap(t *testing.T) {
multisharded, err := CreateVindex(
"etsy_multisharded_hybrid",
"multisharded_test",
map[string]string{"type_id_to_vindex": c.typeIdToVindex})
map[string]string{"type_id_to_vindex": `{"1":"subvindex_a", "2":"subvindex_b"}`})

if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -242,16 +260,14 @@ func TestMultiShardedMap(t *testing.T) {

func TestMultiShardedVerify(t *testing.T) {
cases := []struct {
name string
typeIdToVindex string
rowsColValues [][]sqltypes.Value
ksids [][]byte
expected []bool
shouldErr bool
name string
rowsColValues [][]sqltypes.Value
ksids [][]byte
expected []bool
shouldErr bool
}{
{
"Same subvindex, all ksids map correctly to ids",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -267,7 +283,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Same subvindex, some incorrect ksids",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -283,7 +298,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Different subvindexes, all ksids map correctly to ids",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -299,7 +313,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Different subvindexes, some incorrect ksids",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -315,7 +328,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Different subvindexes, no correct ksids",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -333,7 +345,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Some rows map to subvindexes",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(1), sqltypes.NewInt64(2)},
Expand All @@ -349,7 +360,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"No rows map to subvindexes",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(100), sqltypes.NewInt64(1)},
{sqltypes.NewInt64(200), sqltypes.NewInt64(2)},
Expand All @@ -365,7 +375,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Errors when type_id is negative int",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(-1), sqltypes.NewInt64(1)},
},
Expand All @@ -377,7 +386,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Errors when id is negative int",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewInt64(1), sqltypes.NewInt64(-1)},
},
Expand All @@ -389,7 +397,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Errors when type_id is negative string",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("-1"), sqltypes.NewVarChar("1")},
},
Expand All @@ -401,7 +408,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"Errors when id is negative string",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("1"), sqltypes.NewVarChar("-1")},
},
Expand All @@ -413,7 +419,6 @@ func TestMultiShardedVerify(t *testing.T) {
},
{
"String ids supported",
`{"1":"subvindex_a", "2":"subvindex_b"}`,
[][]sqltypes.Value{
{sqltypes.NewVarChar("1"), sqltypes.NewVarChar("1")},
{sqltypes.NewVarBinary("1"), sqltypes.NewVarBinary("2")},
Expand All @@ -434,7 +439,7 @@ func TestMultiShardedVerify(t *testing.T) {
multisharded, err := CreateVindex(
"etsy_multisharded_hybrid",
"multisharded_test",
map[string]string{"type_id_to_vindex": c.typeIdToVindex})
map[string]string{"type_id_to_vindex": `{"1":"subvindex_a", "2":"subvindex_b"}`})

if err != nil {
t.Fatal(err)
Expand Down
66 changes: 52 additions & 14 deletions go/vt/vtgate/vindexes/vschema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,22 +249,21 @@ func buildKeyspaces(source *vschemapb.SrvVSchema, vschema *VSchema) {

func buildTables(ks *vschemapb.Keyspace, vschema *VSchema, ksvschema *KeyspaceSchema) error {
keyspace := ksvschema.Keyspace
for vname, vindexInfo := range ks.Vindexes {
vindex, err := CreateVindex(vindexInfo.Type, vname, vindexInfo.Params)
if err != nil {
return err
}

// If the keyspace requires explicit routing, don't include it in global routing
if !ks.RequireExplicitRouting {
if _, ok := vschema.uniqueVindexes[vname]; ok {
vschema.uniqueVindexes[vname] = nil
} else {
vschema.uniqueVindexes[vname] = vindex
}
}
ksvschema.Vindexes[vname] = vindex
// CreateVindex will fail if it attempts to create a vindex that depends on another subvindex's existence
// and the subvindex has not been created yet.
// The below retries vindexes that fail to be created due to missing subvindexes.
// Note that this only allows for one layer of dependency between vindexes (failed vindexes are only retried once)
toRetry, err := buildVindexes(ks.Vindexes, ks.RequireExplicitRouting, vschema, ksvschema, true)
if err != nil {
return err
}
// Retry vindexes that failed due to missing subvindexes
_, err = buildVindexes(toRetry, ks.RequireExplicitRouting, vschema, ksvschema, false)
if err != nil {
return err
}

for tname, table := range ks.Tables {
t := &Table{
Name: sqlparser.NewIdentifierCS(tname),
Expand Down Expand Up @@ -403,6 +402,45 @@ func buildTables(ks *vschemapb.Keyspace, vschema *VSchema, ksvschema *KeyspaceSc
return nil
}

func buildVindexes(vindexes map[string]*vschemapb.Vindex, requireExplicitRouting bool, vschema *VSchema, ksvschema *KeyspaceSchema, shouldRetry bool) (map[string]*vschemapb.Vindex, error) {
toRetry := map[string]*vschemapb.Vindex{}
for vname, vindexInfo := range vindexes {
vindex, err := CreateVindex(vindexInfo.Type, vname, vindexInfo.Params)
if err != nil {
if _, ok := err.(*MissingSubvindexError); ok && shouldRetry {

for _, subvindexName := range err.(*MissingSubvindexError).MissingSubvindexes {
// If the vindex depends on itself, return an error without retrying.
if subvindexName == vname {
return nil, fmt.Errorf("circular vindex dependency: Vindex %s depends on itself", vname)
}
// If missing subvindexes arent in `vindexes`, they'll never be instantiated.
// In this case, return an error without retrying.
if _, ok := vindexes[subvindexName]; !ok {
return nil, err
}
}

toRetry[vname] = vindexInfo
continue
} else {
return nil, err
}
}

// If the keyspace requires explicit routing, don't include it in global routing
if !requireExplicitRouting {
if _, ok := vschema.uniqueVindexes[vname]; ok {
vschema.uniqueVindexes[vname] = nil
} else {
vschema.uniqueVindexes[vname] = vindex
}
}
ksvschema.Vindexes[vname] = vindex
}
return toRetry, nil
}

func resolveAutoIncrement(source *vschemapb.SrvVSchema, vschema *VSchema) {
for ksname, ks := range source.Keyspaces {
ksvschema := vschema.Keyspaces[ksname]
Expand Down
Loading

0 comments on commit cd271ef

Please sign in to comment.