Re: [PR] Fix time segment pruning for remote clusters in multi-cluster routing [pinot]

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #18855:
URL: https://github.com/apache/pinot/pull/18855#discussion_r3567155444


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -130,4 +146,26 @@ public synchronized void refreshSegment(String segment) {
       }
     }
   }
+
+  /**

Review Comment:
   (minor) Use markdown style for new added javadocs. Same for other places



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/TimeSegmentPruner.java:
##########
@@ -107,12 +107,16 @@ private Interval extractIntervalFromSegmentZKMetaZNRecord(String segment, @Nulla
   @Override
   public synchronized void onAssignmentChange(IdealState idealState, ExternalView externalView,
       Set<String> onlineSegments, List<String> pulledSegments, List<ZNRecord> znRecords) {
-    // NOTE: We don't update all the segment ZK metadata for every external view change, but only the new added/removed
-    //       ones. The refreshed segment ZK metadata change won't be picked up.
     for (int idx = 0; idx < pulledSegments.size(); idx++) {
       String segment = pulledSegments.get(idx);
       ZNRecord zNrecord = znRecords.get(idx);
-      _intervalMap.computeIfAbsent(segment, k -> extractIntervalFromSegmentZKMetaZNRecord(k, zNrecord));
+      // Always update segments that have DEFAULT_INTERVAL, which covers two cases:
+      // 1. New segments not yet in the map
+      // 2. Segments that transitioned from CONSUMING (DEFAULT_INTERVAL) to COMMITTED (valid time range)
+      Interval existing = _intervalMap.get(segment);

Review Comment:
   We can use `compute` to reduce one map lookup



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -82,7 +85,11 @@ public void init(IdealState idealState, ExternalView externalView, Set<String> o
           listener.init(idealState, externalView, segments, znRecords);
         }
         for (int i = 0; i < numSegments; i++) {
-          if (znRecords.get(i) != null) {
+          // Only cache segments that are both non-consuming in EV AND have a committed ZNRecord.
+          // If a segment is ONLINE in EV but its ZNRecord still has startTime=-1 (brief window between

Review Comment:
   This shouldn't be possible. During segment commit, controller updates ZNRecord first, then update IS. EV change only happens after IS change through state transition.
   I guess the intention here is to just not cache consuming segment state? But that will result in consuming segment not being pruned.



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -130,4 +146,26 @@ public synchronized void refreshSegment(String segment) {
       }
     }
   }
+
+  /**
+   * Returns true if the ZNRecord represents a committed segment with a valid startTime.
+   * A null ZNRecord or one with startTime=-1 (consuming, or briefly inconsistent after commit)
+   * should not be cached — the segment will be re-fetched on the next onAssignmentChange.
+   */
+  private static boolean isCommittedZNRecord(@Nullable ZNRecord znRecord) {
+    return znRecord != null && znRecord.getLongField(CommonConstants.Segment.START_TIME, -1L) >= 0L;
+  }
+
+  /**
+   * Returns true if the segment is in CONSUMING state on any server in the ExternalView.
+   * Such segments should not be cached in {@code _onlineSegmentsCached} — they will be re-evaluated
+   * on the next ExternalView change, at which point they will have transitioned to ONLINE (committed).
+   */
+  private static boolean isConsumingInExternalView(ExternalView externalView, String segment) {
+    if (externalView == null) {

Review Comment:
   (minor) No need to check. We follow the annotation convention, and external view can never be null



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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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