[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #4338: Within a partition, only allow querying the first CONSUMING segment for the real-time table routing

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

mcvsubbu commented on a change in pull request #4338: Within a partition, only allow querying the first CONSUMING segment for the real-time table routing
URL: https://github.com/apache/incubator-pinot/pull/4338#discussion_r295116979
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/builder/LowLevelRoutingTableBuilderUtil.java
 ##########
 @@ -32,59 +32,49 @@
 public class LowLevelRoutingTableBuilderUtil {
 
   /**
-   * Compute the map of allowed 'consuming' segments for each partition.
+   * Compute the map of allowed CONSUMING segments for each partition for routing purpose.
+   * <p>Within a partition, we only allow querying the first CONSUMING segment (segment with instances in CONSUMING
+   * state) for the following reasons:
+   * <ul>
+   *   <li>
+   *     If within a partition, there are multiple CONSUMING segments (typically caused by the delay of CONSUMING to
+   *     ONLINE state transition), we can only query the first CONSUMING segment because it might contain records that
+   *     overlapped with the records in the next segment (over-consumed).
+   *   </li>
+   *   <li>
+   *     If the instance states for a segment is partial ONLINE and partial CONSUMING (some instances finished the
+   *     CONSUMING to ONLINE state transition, others didn't), we count the segment as CONSUMING segment (most likely
+   *     the first CONSUMING segment because it is already committed and is performing the CONSUMING to ONLINE state
+   *     transition). If we don't count the segment as CONSUMING segment, then this segment is not allowed to be in the
+   *     CONSUMING state for routing purpose, and we will route all queries to the ONLINE instances which can
+   *     potentially overwhelm instance.
+   *   </li>
+   *   <li>
+   *     It is possible that the latest CONSUMING segment is not allowed for routing purpose and we won't query it, but
+   *     it should only last for a short period of time. Once the older CONSUMING segment becomes ONLINE (all instances
+   *     finished the CONSUMING to ONLINE state transition), the latest CONSUMING segment will become the first
+   *     CONSUMING segment and will be allowed for routing purpose.
+   *   </li>
+   * </ul>
    *
    * @param externalView helix external view
    * @param sortedSegmentsByPartition map of partition to sorted set of segment names.
    * @return map of allowed consuming segment for each partition for routing.
    */
   public static Map<String, SegmentName> getAllowedConsumingStateSegments(ExternalView externalView,
       Map<String, SortedSet<SegmentName>> sortedSegmentsByPartition) {
-    Map<String, SegmentName> allowedSegmentInConsumingStateByPartition = new HashMap<>();
-    for (String partition : sortedSegmentsByPartition.keySet()) {
-      SortedSet<SegmentName> sortedSegmentsForPartition = sortedSegmentsByPartition.get(partition);
-      SegmentName lastAllowedSegmentInConsumingState = null;
-
+    Map<String, SegmentName> allowedConsumingSegments = new HashMap<>();
+    for (Map.Entry<String, SortedSet<SegmentName>> entry : sortedSegmentsByPartition.entrySet()) {
+      String partitionId = entry.getKey();
+      SortedSet<SegmentName> sortedSegmentsForPartition = entry.getValue();
       for (SegmentName segmentName : sortedSegmentsForPartition) {
-        Map<String, String> helixPartitionState = externalView.getStateMap(segmentName.getSegmentName());
-        boolean allInConsumingState = true;
-        int replicasInConsumingState = 0;
-
-        // Only keep the segment if all replicas have it in CONSUMING state
-        for (String externalViewState : helixPartitionState.values()) {
-          // Ignore ERROR state
-          if (externalViewState
-              .equalsIgnoreCase(CommonConstants.Helix.StateModel.RealtimeSegmentOnlineOfflineStateModel.ERROR)) {
-            continue;
-          }
-
-          // Not all segments are in CONSUMING state, therefore don't consider the last segment assignable to CONSUMING
-          // replicas
-          if (externalViewState
-              .equalsIgnoreCase(CommonConstants.Helix.StateModel.RealtimeSegmentOnlineOfflineStateModel.ONLINE)) {
-            allInConsumingState = false;
-            break;
-          }
-
-          // Otherwise count the replica as being in CONSUMING state
-          if (externalViewState
-              .equalsIgnoreCase(CommonConstants.Helix.StateModel.RealtimeSegmentOnlineOfflineStateModel.CONSUMING)) {
-            replicasInConsumingState++;
-          }
-        }
-
-        // If all replicas have this segment in consuming state (and not all of them are in ERROR state), then pick this
-        // segment to be the last allowed segment to be in CONSUMING state
-        if (allInConsumingState && 0 < replicasInConsumingState) {
-          lastAllowedSegmentInConsumingState = segmentName;
+        if (externalView.getStateMap(segmentName.getSegmentName())
 
 Review comment:
   A revers iterator, if possible, will get us here faster most of the time? the logic may be a bit complex...

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