ankitsultana commented on code in PR #16034:
URL: https://github.com/apache/pinot/pull/16034#discussion_r2136837250
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -380,6 +381,23 @@ public long getIndexCreationTime() {
return _creationTime;
}
+ /**
+ * Returns the ZooKeeper creation time for upsert consistency.
Review Comment:
Can you also call out that this refers to the time set by the controller
when creating new consuming segment?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1146,4 +1148,21 @@ public Set<String> getNewlyAddedSegments() {
}
return Collections.emptySet();
}
+
+ /**
+ * Gets the authoritative creation time for upsert comparison.
+ * Uses ZK creation time if available for consistency across replicas,
otherwise falls back to local creation time.
+ */
+ protected long getAuthoritativeCreationTime(IndexSegment segment) {
+ SegmentMetadata segmentMetadata = segment.getSegmentMetadata();
+ if (segmentMetadata instanceof SegmentMetadataImpl) {
Review Comment:
why the instanceof check? Are you trying to avoid mutable segment? if so,
prefer `isMutableSegment`.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -832,7 +853,12 @@ public void replaceConsumingSegment(String segmentName)
File indexDir = new File(_indexDir, segmentName);
// Get a new index loading config with latest table config and schema to
load the segment
IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig();
- addSegment(ImmutableSegmentLoader.load(indexDir, indexLoadingConfig,
_segmentOperationsThrottler));
+ ImmutableSegment immutableSegment =
+ ImmutableSegmentLoader.load(indexDir, indexLoadingConfig,
_segmentOperationsThrottler);
+
+ // Fetch ZK metadata to pass to addSegment so that ZK creation time is set
automatically
+ SegmentZKMetadata zkMetadata = fetchZKMetadataNullable(segmentName);
Review Comment:
`replaceConsumingSegment` is only called in RSDM, which has a member var
with SegmentZKMetadata.
Fetching it here adds an extra ZK call which is avoidable.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
