From b06292ffe044bddcfc05f7ed252089a52604c54f Mon Sep 17 00:00:00 2001 From: Rafael Chacon Date: Thu, 23 Sep 2021 09:35:19 -0700 Subject: [PATCH 1/3] Fix to make sure all keyrange comparisons go through the key.Package * We ran into some issues related to padding. This is another form of the bug fixed in here: https://github.com/vitessio/vitess/pull/8296 Signed-off-by: Rafael Chacon --- go/vt/key/key.go | 8 +++ go/vt/key/key_test.go | 114 +++++++++++++++++++------------------ go/vt/topo/srv_keyspace.go | 3 +- 3 files changed, 68 insertions(+), 57 deletions(-) diff --git a/go/vt/key/key.go b/go/vt/key/key.go index 523d154042b..1ae19205569 100644 --- a/go/vt/key/key.go +++ b/go/vt/key/key.go @@ -242,6 +242,14 @@ func KeyRangeStartEqual(left, right *topodatapb.KeyRange) bool { return bytes.Equal(addPadding(left.Start), addPadding(right.Start)) } +// KeyRangeIsContiguous returns true if left.End equals right.End (i.e they are contigious) +func KeyRangeIsContiguous(left, right *topodatapb.KeyRange) bool { + if left == nil || right == nil { + return false + } + return bytes.Equal(addPadding(left.End), addPadding(right.Start)) +} + // KeyRangeEndEqual returns true if both key ranges have the same end func KeyRangeEndEqual(left, right *topodatapb.KeyRange) bool { if left == nil { diff --git a/go/vt/key/key_test.go b/go/vt/key/key_test.go index 49babeff3f5..631029051d8 100644 --- a/go/vt/key/key_test.go +++ b/go/vt/key/key_test.go @@ -212,20 +212,6 @@ func TestKeyRangeAdd(t *testing.T) { out: "40-c0", ok: true, }} - stringToKeyRange := func(spec string) *topodatapb.KeyRange { - if spec == "" { - return nil - } - parts := strings.Split(spec, "-") - if len(parts) != 2 { - panic("invalid spec") - } - kr, err := ParseKeyRangeParts(parts[0], parts[1]) - if err != nil { - panic(err) - } - return kr - } keyRangeToString := func(kr *topodatapb.KeyRange) string { if kr == nil { return "" @@ -271,20 +257,6 @@ func TestKeyRangeEndEqual(t *testing.T) { second: "-8000", out: true, }} - stringToKeyRange := func(spec string) *topodatapb.KeyRange { - if spec == "" { - return nil - } - parts := strings.Split(spec, "-") - if len(parts) != 2 { - panic("invalid spec") - } - kr, err := ParseKeyRangeParts(parts[0], parts[1]) - if err != nil { - panic(err) - } - return kr - } for _, tcase := range testcases { first := stringToKeyRange(tcase.first) @@ -326,20 +298,6 @@ func TestKeyRangeStartEqual(t *testing.T) { second: "-8000", out: true, }} - stringToKeyRange := func(spec string) *topodatapb.KeyRange { - if spec == "" { - return nil - } - parts := strings.Split(spec, "-") - if len(parts) != 2 { - panic("invalid spec") - } - kr, err := ParseKeyRangeParts(parts[0], parts[1]) - if err != nil { - panic(err) - } - return kr - } for _, tcase := range testcases { first := stringToKeyRange(tcase.first) @@ -377,25 +335,56 @@ func TestKeyRangeEqual(t *testing.T) { second: "-8000", out: true, }} - stringToKeyRange := func(spec string) *topodatapb.KeyRange { - if spec == "" { - return nil - } - parts := strings.Split(spec, "-") - if len(parts) != 2 { - panic("invalid spec") - } - kr, err := ParseKeyRangeParts(parts[0], parts[1]) - if err != nil { - panic(err) + + for _, tcase := range testcases { + first := stringToKeyRange(tcase.first) + second := stringToKeyRange(tcase.second) + out := KeyRangeEqual(first, second) + if out != tcase.out { + t.Fatalf("KeyRangeEqual(%q, %q) expected %t, got %t", tcase.first, tcase.second, tcase.out, out) } - return kr } +} + +func TestKeyRangeIsContiguous(t *testing.T) { + testcases := []struct { + first string + second string + out bool + }{{ + first: "-40", + second: "40-80", + out: true, + }, { + first: "40-80", + second: "-40", + out: false, + }, { + first: "-", + second: "-40", + out: true, + }, { + first: "40-80", + second: "c0-", + out: false, + }, { + first: "40-80", + second: "80-c0", + out: true, + }, { + first: "40-80", + second: "8000000000000000-c000000000000000", + out: true, + }, { + first: "4000000000000000-8000000000000000", + second: "80-c0", + out: true, + }} for _, tcase := range testcases { first := stringToKeyRange(tcase.first) second := stringToKeyRange(tcase.second) - out := KeyRangeEqual(first, second) + out := KeyRangeIsContiguous(first, second) if out != tcase.out { t.Fatalf("KeyRangeEqual(%q, %q) expected %t, got %t", tcase.first, tcase.second, tcase.out, out) } @@ -815,3 +804,18 @@ func TestShardCalculatorForShardsGreaterThan512(t *testing.T) { assert.Equal(t, want, got[511], "Invalid mapping for a 512-shard keyspace. Expected %v, got %v", want, got[511]) } + +func stringToKeyRange(spec string) *topodatapb.KeyRange { + if spec == "" { + return nil + } + parts := strings.Split(spec, "-") + if len(parts) != 2 { + panic("invalid spec") + } + kr, err := ParseKeyRangeParts(parts[0], parts[1]) + if err != nil { + panic(err) + } + return kr +} diff --git a/go/vt/topo/srv_keyspace.go b/go/vt/topo/srv_keyspace.go index 830cc06eb16..78e87519f34 100644 --- a/go/vt/topo/srv_keyspace.go +++ b/go/vt/topo/srv_keyspace.go @@ -17,7 +17,6 @@ limitations under the License. package topo import ( - "bytes" "encoding/hex" "fmt" "path" @@ -681,7 +680,7 @@ func OrderAndCheckPartitions(cell string, srvKeyspace *topodatapb.SrvKeyspace) e // this is the custom sharding case, all KeyRanges must be nil continue } - if !bytes.Equal(currShard.KeyRange.End, nextShard.KeyRange.Start) { + if !key.KeyRangeIsContiguous(currShard.KeyRange, nextShard.KeyRange) { return fmt.Errorf("non-contiguous KeyRange values for %v in cell %v at shard %v to %v: %v != %v", tabletType, cell, i, i+1, hex.EncodeToString(currShard.KeyRange.End), hex.EncodeToString(nextShard.KeyRange.Start)) } } From 476ba905a3ccf0b5d8d1f04da2130e11acd38c88 Mon Sep 17 00:00:00 2001 From: Rafael Chacon Date: Thu, 23 Sep 2021 10:39:21 -0700 Subject: [PATCH 2/3] Address code review * Better description for the documentation. * For completeness, compare nil ranges in a similar a way as in the other functions Signed-off-by: Rafael Chacon --- go/vt/key/key.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/go/vt/key/key.go b/go/vt/key/key.go index 1ae19205569..f3b0669db55 100644 --- a/go/vt/key/key.go +++ b/go/vt/key/key.go @@ -242,10 +242,14 @@ func KeyRangeStartEqual(left, right *topodatapb.KeyRange) bool { return bytes.Equal(addPadding(left.Start), addPadding(right.Start)) } -// KeyRangeIsContiguous returns true if left.End equals right.End (i.e they are contigious) +// KeyRangeIsContiguous returns true if the end of the left key range exactly +// matches the start of the right key range (i.e they are contigious) func KeyRangeIsContiguous(left, right *topodatapb.KeyRange) bool { - if left == nil || right == nil { - return false + if left == nil { + return right == nil || (len(right.Start) == 0 && len(right.End) == 0) + } + if right == nil { + return len(left.Start) == 0 && len(left.End) == 0 } return bytes.Equal(addPadding(left.End), addPadding(right.Start)) } From 157961161d59caddc3f8170551f4703ee8313748 Mon Sep 17 00:00:00 2001 From: Rafael Chacon Date: Thu, 23 Sep 2021 11:49:20 -0700 Subject: [PATCH 3/3] Fix typo in tests, also rename method to be more consistent with the package naming conventions Signed-off-by: Rafael Chacon --- go/vt/key/key.go | 4 ++-- go/vt/key/key_test.go | 6 +++--- go/vt/topo/srv_keyspace.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go/vt/key/key.go b/go/vt/key/key.go index f3b0669db55..1c315866b7f 100644 --- a/go/vt/key/key.go +++ b/go/vt/key/key.go @@ -242,9 +242,9 @@ func KeyRangeStartEqual(left, right *topodatapb.KeyRange) bool { return bytes.Equal(addPadding(left.Start), addPadding(right.Start)) } -// KeyRangeIsContiguous returns true if the end of the left key range exactly +// KeyRangeContiguous returns true if the end of the left key range exactly // matches the start of the right key range (i.e they are contigious) -func KeyRangeIsContiguous(left, right *topodatapb.KeyRange) bool { +func KeyRangeContiguous(left, right *topodatapb.KeyRange) bool { if left == nil { return right == nil || (len(right.Start) == 0 && len(right.End) == 0) } diff --git a/go/vt/key/key_test.go b/go/vt/key/key_test.go index 631029051d8..639b81c5f18 100644 --- a/go/vt/key/key_test.go +++ b/go/vt/key/key_test.go @@ -346,7 +346,7 @@ func TestKeyRangeEqual(t *testing.T) { } } -func TestKeyRangeIsContiguous(t *testing.T) { +func TestKeyRangeContiguous(t *testing.T) { testcases := []struct { first string second string @@ -384,9 +384,9 @@ func TestKeyRangeIsContiguous(t *testing.T) { for _, tcase := range testcases { first := stringToKeyRange(tcase.first) second := stringToKeyRange(tcase.second) - out := KeyRangeIsContiguous(first, second) + out := KeyRangeContiguous(first, second) if out != tcase.out { - t.Fatalf("KeyRangeEqual(%q, %q) expected %t, got %t", tcase.first, tcase.second, tcase.out, out) + t.Fatalf("KeyRangeContiguous(%q, %q) expected %t, got %t", tcase.first, tcase.second, tcase.out, out) } } } diff --git a/go/vt/topo/srv_keyspace.go b/go/vt/topo/srv_keyspace.go index 78e87519f34..264b01a3edc 100644 --- a/go/vt/topo/srv_keyspace.go +++ b/go/vt/topo/srv_keyspace.go @@ -680,7 +680,7 @@ func OrderAndCheckPartitions(cell string, srvKeyspace *topodatapb.SrvKeyspace) e // this is the custom sharding case, all KeyRanges must be nil continue } - if !key.KeyRangeIsContiguous(currShard.KeyRange, nextShard.KeyRange) { + if !key.KeyRangeContiguous(currShard.KeyRange, nextShard.KeyRange) { return fmt.Errorf("non-contiguous KeyRange values for %v in cell %v at shard %v to %v: %v != %v", tabletType, cell, i, i+1, hex.EncodeToString(currShard.KeyRange.End), hex.EncodeToString(nextShard.KeyRange.Start)) } }