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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
