kvclient: sort by region before sorting by latency

Follower reads attempt to be efficient by sorting to the nearest
follower first. The nearest follower can be either the closest by
latency or by locality. There is a complex relationship between dollar
cost, latency and throughput for different nodes, and just sorting by
latency is not ideal in many cases. This PR changes the behavior to
first sort by the locality definition and then the latency. In most
cases this won't make a difference to the user, but in some cases this
behavior can be better.

Epic: none

Release note (performance improvement): Sorting by locality can improve
the performance of follower reads.

pkg/kv/kvclient/kvcoord/replica_slice.go

@@ -228,18 +228,29 @@ func (rs ReplicaSlice) OptimizeReplicaOrder(
 			return false // j < i
 		}
 
+		// If we have locality defined use this first.
+		attrMatchI := localityMatch(locality.Tiers, rs[i].Tiers)
+		attrMatchJ := localityMatch(locality.Tiers, rs[j].Tiers)
+		// Longer locality matches sort first (the assumption is that
+		// they'll have better latencies).
+		if attrMatchI != attrMatchJ {
+			return attrMatchI > attrMatchJ
+		}
+
+		// These nodes are otherwise equal, choose the one that has a lower
+		// latency to us.
 		if latencyFn != nil {
 			latencyI, okI := latencyFn(rs[i].NodeID)
 			latencyJ, okJ := latencyFn(rs[j].NodeID)
 			if okI && okJ {
 				return latencyI < latencyJ
 			}
 		}
-		attrMatchI := localityMatch(locality.Tiers, rs[i].Tiers)
-		attrMatchJ := localityMatch(locality.Tiers, rs[j].Tiers)
-		// Longer locality matches sort first (the assumption is that
-		// they'll have better latencies).
-		return attrMatchI > attrMatchJ
+		// We need to choose one node, so for consistent sorting, always choose
+		// the node with the lower node id. Note that on real systems we almost
+		// never get here since the latencyFn will be defined and we return a
+		// latency for both the nodes.
+		return rs[i].NodeID < rs[j].NodeID
 	})
 }
 

pkg/kv/kvclient/kvcoord/replica_slice_test.go

@@ -218,7 +218,24 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) {
 			expOrdered: []roachpb.NodeID{1, 2, 4, 3},
 		},
 		{
-			name:     "order by latency",
+			name:     "order by latency only",
+			nodeID:   1,
+			locality: locality(t, []string{"country=us"}),
+			latencies: map[roachpb.NodeID]time.Duration{
+				2: time.Hour,
+				3: time.Minute,
+				4: time.Second,
+			},
+			slice: ReplicaSlice{
+				info(t, 2, 2, []string{"country=us"}),
+				info(t, 4, 4, []string{"country=us"}),
+				info(t, 4, 44, []string{"country=us"}),
+				info(t, 3, 3, []string{"country=us"}),
+			},
+			expOrdered: []roachpb.NodeID{4, 3, 2},
+		},
+		{
+			name:     "order by locality than latency",
 			nodeID:   1,
 			locality: locality(t, []string{"country=us", "region=west", "city=la"}),
 			latencies: map[roachpb.NodeID]time.Duration{
@@ -232,7 +249,7 @@ func TestReplicaSliceOptimizeReplicaOrder(t *testing.T) {
 				info(t, 4, 44, []string{"country=us", "region=east", "city=ny"}),
 				info(t, 3, 3, []string{"country=uk", "city=london"}),
 			},
-			expOrdered: []roachpb.NodeID{4, 3, 2},
+			expOrdered: []roachpb.NodeID{2, 4, 3},
 		},
 		{
 			// Test that replicas on the local node sort first, regardless of factors