From 059aebfe333fcfb5c6f23b3928d4dc073526e7ed Mon Sep 17 00:00:00 2001 From: Andrei Matei Date: Thu, 29 Apr 2021 12:39:52 -0400 Subject: [PATCH] kvcoord: prepare rep slice test for multiple replicas on node TestReplicaSliceOptimizeReplicaOrder did not support test cases with multiple replicas on a node, as the sort order between these replicas is not deterministic. Now it does, as the test is taught to ignore the ordering between such duplicate replicas. Release note: None --- pkg/kv/kvclient/kvcoord/BUILD.bazel | 1 + pkg/kv/kvclient/kvcoord/replica_slice_test.go | 47 ++++++++++--------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/BUILD.bazel b/pkg/kv/kvclient/kvcoord/BUILD.bazel index 9d3adc485b2e..ebd4d1a22d97 100644 --- a/pkg/kv/kvclient/kvcoord/BUILD.bazel +++ b/pkg/kv/kvclient/kvcoord/BUILD.bazel @@ -135,6 +135,7 @@ go_test( "//pkg/util/netutil", "//pkg/util/randutil", "//pkg/util/retry", + "//pkg/util/shuffle", "//pkg/util/stop", "//pkg/util/syncutil", "//pkg/util/tracing", diff --git a/pkg/kv/kvclient/kvcoord/replica_slice_test.go b/pkg/kv/kvclient/kvcoord/replica_slice_test.go index d1942bb6e075..4a861b9e6fc9 100644 --- a/pkg/kv/kvclient/kvcoord/replica_slice_test.go +++ b/pkg/kv/kvclient/kvcoord/replica_slice_test.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/shuffle" "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" ) @@ -181,9 +182,13 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) { name string node *roachpb.NodeDescriptor // map from node address (see nodeDesc()) to latency to that node. - latencies map[string]time.Duration - slice ReplicaSlice - expOrdered ReplicaSlice + latencies map[string]time.Duration + slice ReplicaSlice + // expOrder is the expected order in which the replicas sort. Replicas are + // only identified by their node. If multiple replicas are on different + // stores of the same node, the node only appears once in this list (as the + // ordering between replicas on the same node is not deterministic). + expOrdered []roachpb.NodeID }{ { name: "order by locality matching", @@ -191,13 +196,10 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) { slice: ReplicaSlice{ info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), info(t, 3, 3, []string{"country=uk", "city=london"}), + info(t, 3, 33, []string{"country=uk", "city=london"}), info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), }, - expOrdered: ReplicaSlice{ - info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), - info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), - info(t, 3, 3, []string{"country=uk", "city=london"}), - }, + expOrdered: []roachpb.NodeID{2, 4, 3}, }, { name: "order by locality matching, put node first", @@ -206,14 +208,10 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) { info(t, 1, 1, []string{"country=us", "region=west", "city=la"}), info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), info(t, 3, 3, []string{"country=uk", "city=london"}), + info(t, 3, 33, []string{"country=uk", "city=london"}), info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), }, - expOrdered: ReplicaSlice{ - info(t, 1, 1, []string{"country=us", "region=west", "city=la"}), - info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), - info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), - info(t, 3, 3, []string{"country=uk", "city=london"}), - }, + expOrdered: []roachpb.NodeID{1, 2, 4, 3}, }, { name: "order by latency", @@ -226,13 +224,10 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) { slice: ReplicaSlice{ info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), + info(t, 4, 44, []string{"country=us", "region=east", "city=ny"}), info(t, 3, 3, []string{"country=uk", "city=london"}), }, - expOrdered: ReplicaSlice{ - info(t, 4, 4, []string{"country=us", "region=east", "city=ny"}), - info(t, 3, 3, []string{"country=uk", "city=london"}), - info(t, 2, 2, []string{"country=us", "region=west", "city=sf"}), - }, + expOrdered: []roachpb.NodeID{4, 3, 2}, }, } for _, test := range testCases { @@ -244,9 +239,19 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) { return lat, ok } } + // Randomize the input order, as it's not supposed to matter. + shuffle.Shuffle(test.slice) test.slice.OptimizeReplicaOrder(test.node, latencyFn) - if !reflect.DeepEqual(test.slice, test.expOrdered) { - t.Errorf("expected order %+v; got %+v", test.expOrdered, test.slice) + var sortedNodes []roachpb.NodeID + sortedNodes = append(sortedNodes, test.slice[0].NodeID) + for i := 1; i < len(test.slice); i++ { + l := len(sortedNodes) + if nid := test.slice[i].NodeID; nid != sortedNodes[l-1] { + sortedNodes = append(sortedNodes, nid) + } + } + if !reflect.DeepEqual(sortedNodes, test.expOrdered) { + t.Errorf("expected node order %+v; got %+v", test.expOrdered, test.slice) } }) }