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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
