[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5337: Track unavailable segments in InstanceSelector

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

mcvsubbu commented on a change in pull request #5337:
URL: https://github.com/apache/incubator-pinot/pull/5337#discussion_r420981619



##########
File path: pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java
##########
@@ -394,13 +422,66 @@ public void testInstanceSelector() {
     expectedBalancedInstanceSelectorResult.put(segment2, instance1);
     expectedBalancedInstanceSelectorResult.put(segment3, instance3);
     expectedBalancedInstanceSelectorResult.put(segment4, instance0);
-    assertEquals(balancedInstanceSelector.select(brokerRequest, segments), expectedBalancedInstanceSelectorResult);
+    selectionResult = balancedInstanceSelector.select(brokerRequest, segments);
+    assertEquals(selectionResult.getSegmentToInstanceMap(), expectedBalancedInstanceSelectorResult);
+    assertTrue(selectionResult.getUnavailableSegments().isEmpty());
     expectedReplicaGroupInstanceSelectorResult = new HashMap<>();
     expectedReplicaGroupInstanceSelectorResult.put(segment1, instance2);
     expectedReplicaGroupInstanceSelectorResult.put(segment2, instance3);
     expectedReplicaGroupInstanceSelectorResult.put(segment3, instance3);
     expectedReplicaGroupInstanceSelectorResult.put(segment4, instance2);
-    assertEquals(replicaGroupInstanceSelector.select(brokerRequest, segments),
-        expectedReplicaGroupInstanceSelectorResult);
+    selectionResult = replicaGroupInstanceSelector.select(brokerRequest, segments);
+    assertEquals(selectionResult.getSegmentToInstanceMap(), expectedReplicaGroupInstanceSelectorResult);
+    assertTrue(selectionResult.getUnavailableSegments().isEmpty());
+  }
+
+  @Test
+  public void testUnavailableSegments() {

Review comment:
       Good to iterate this test a few times, with realtime table and with CONSUMING segments as well

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
##########
@@ -85,94 +88,147 @@ public void onInstancesChange(Set<String> enabledInstances, List<String> changed
       return;
     }
 
-    // Update the map from segment to enabled instances
+    // Update the map from segment to enabled ONLINE/CONSUMING instances and set of unavailable segments (no enabled
+    // instance or all enabled instances are in ERROR state)
     // NOTE: We can directly modify the map because we will only update the values without changing the map entries.
     // Because the map is marked as volatile, the running queries (already accessed the map) might use the enabled
     // instances either before or after the change, which is okay; the following queries (not yet accessed the map) will
     // get the updated value.
-    Map<String, List<String>> segmentToInstancesMap = _segmentToInstancesMap;
+    Map<String, List<String>> segmentToOnlineInstancesMap = _segmentToOnlineInstancesMap;
+    Map<String, List<String>> segmentToOfflineInstancesMap = _segmentToOfflineInstancesMap;
     Map<String, List<String>> segmentToEnabledInstancesMap = _segmentToEnabledInstancesMap;
+    Set<String> currentUnavailableSegments = _unavailableSegments;

Review comment:
       we dont need this variable, we can use `_unavailableSegments` in line 110 right?




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



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