[GitHub] [incubator-pinot] snleee commented on a change in pull request #4119: Improve partition aware routing when a server is down.

[GitBox <gi...@apache.org>]

snleee commented on a change in pull request #4119: Improve partition aware routing when a server is down.
URL: https://github.com/apache/incubator-pinot/pull/4119#discussion_r275922913
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/builder/BasePartitionAwareRoutingTableBuilder.java
 ##########
 @@ -101,40 +101,59 @@ public void init(Configuration configuration, TableConfig tableConfig, ZkHelixPr
     Map<String, List<String>> routingTable = new HashMap<>();
     SegmentPrunerContext prunerContext = new SegmentPrunerContext(request.getBrokerRequest());
 
-    // 1. Randomly pick a replica id
-    int replicaId = _random.nextInt(_numReplicas);
+    // Shuffle the replica group id pool in order to satisfy:
+    // 1. Pick a replica group in an evenly distributed fashion
+    // 2. When a server is not available, the request should be distributed evenly among other available servers.
+    int[] replicaGroupIdPool = new int[_numReplicas];
+    for (int i = 0; i < _numReplicas; i++) {
+      replicaGroupIdPool[i] = i;
+    }
+    shuffleArray(replicaGroupIdPool);
+
     for (String segmentName : segmentsToQuery) {
       SegmentZKMetadata segmentZKMetadata = _segmentToZkMetadataMapping.get(segmentName);
 
       // 2a. Check if the segment can be pruned
       boolean segmentPruned = (segmentZKMetadata != null) && _pruner.prune(segmentZKMetadata, prunerContext);
 
       if (!segmentPruned) {
-        // 2b. Segment cannot be pruned. Assign the segment to a server with the replica id picked above.
+        // 2b. Segment cannot be pruned. Assign the segment to a server based on the replica group id pool
         Map<Integer, String> replicaIdToServerMap = segmentToReplicaToServerMap.get(segmentName);
-        String serverName = replicaIdToServerMap.get(replicaId);
-
-        // When the server is not available with this replica id, we need to pick another available server.
-        if (serverName == null) {
-          if (!replicaIdToServerMap.isEmpty()) {
-            serverName = replicaIdToServerMap.values().iterator().next();
-          } else {
-            // No server is found for this segment
-            continue;
+
+        String serverName = null;
+        for (int i = 0; i < _numReplicas; i++) {
+          serverName = replicaIdToServerMap.get(replicaGroupIdPool[i]);
 
 Review comment:
   Yeah, this code is in the query execution path on the broker where it creates the routing table on the fly for each query.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org