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:
us...@infra.apache.org



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

Reply via email to