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

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

sunithabeeram 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_r276065806
 
 

 ##########
 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]);
+          // If a server is found, update routing table for the current segment
+          if (serverName != null) {
+            break;
           }
         }
-        List<String> segmentsForServer = routingTable.get(serverName);
-        if (segmentsForServer == null) {
-          segmentsForServer = new ArrayList<>();
-          routingTable.put(serverName, segmentsForServer);
+
+        if (serverName != null) {
+          List<String> segmentsForServer = routingTable.computeIfAbsent(serverName, k -> new ArrayList<>());
+          segmentsForServer.add(segmentName);
+        } else {
+          // No server is found for this segment if the code reach here
 
 Review comment:
   Why are we not calling handleNoServingHost() here?

----------------------------------------------------------------
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