[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5728: Add segment lineage based segment selector

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

mayankshriv commented on a change in pull request #5728:
URL: https://github.com/apache/incubator-pinot/pull/5728#discussion_r460408329



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentselector/OfflineSegmentSelector.java
##########
@@ -30,10 +30,17 @@
  * Segment selector for offline table.
  */
 public class OfflineSegmentSelector implements SegmentSelector {
+  private final SegmentLineageBasedSegmentSelector _segmentLineageBasedSegmentSelector;

Review comment:
       This seems a bit confusing:
   1. One segment selector class contains another segment segment selector.
   2. Even though both are named SegmentSelector, only one of them implements the SegmentSelector interface.
   
   I think SegmentLineageBasedSegmentSelector needs to be renamed, since it is not really implementing the interface.

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentselector/OfflineSegmentSelector.java
##########
@@ -42,7 +49,14 @@ public void onExternalViewChange(ExternalView externalView, Set<String> onlineSe
     // TODO: for new added segments, before all replicas are up, consider not selecting them to avoid causing
     //       hotspot servers
 
-    _segments = Collections.unmodifiableList(new ArrayList<>(onlineSegments));
+
+    // Update segment lineage based segment selector
+    _segmentLineageBasedSegmentSelector.onExternalViewChange();
+
+    // Compute the intersection of segments to process from both offline and segment lineage based segment selectors.
+    List<String> segmentsToProcess =
+        new ArrayList<>(_segmentLineageBasedSegmentSelector.computeSegmentsToProcess(onlineSegments));

Review comment:
       I am unclear on the extensibility of the code in future. Will we add more selectors and cascade them in future, for example versionBasedSelector/Pruner?




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