xiangfu0 commented on code in PR #19730:
URL: https://github.com/apache/pinot/pull/19730#discussion_r4278291024
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -853,10 +855,42 @@ private String createNewSegmentMetadata(TableConfig
tableConfig,
LOGGER.info(
"Skipping creation of new segment metadata after segment: {} during
commit. Reason: Partition ID: {} not "
+ "found in upstream metadata.", committingSegmentName,
committingSegmentPartitionGroupId);
+ setPartitionEndOfLifeGauge(realtimeTableName,
committingSegmentPartitionGroupId, true);
Review Comment:
**MINOR.** This commit-path mark has no leadership re-check, so if
leadership for the table transfers while this request is in flight,
`computeIfAbsent` here can re-create the table entry and re-emit the gauge
right after `nonLeaderCleanup` already cleared it, leaving a stale gauge on a
non-leader until restart. This mirrors the existing
`cleanupTotalDocumentCountGauge`/`removeTableMeter` cleanups, so it's
consistent with the current pattern — just flagging the inherent window.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1940,10 +1987,18 @@ && new
LLCSegmentName(segmentEntry.getKey()).getPartitionGroupId() == partitionI
currentTimeMs, instancePartitions, numPartitions,
numReplicas);
updateInstanceStatesForNewConsumingSegment(instanceStatesMap, null,
newSegmentName, segmentAssignment,
instancePartitionsMap);
+ partitionIdToEndOfLife.put(partitionId, false);
}
}
}
+ // Drop the gauges of partitions that no longer have segments, e.g. an
ended partition removed by retention
+ Set<Integer> partitionIdsToKeep = new
HashSet<>(latestSegmentZKMetadataMap.keySet());
+ partitionIdsToKeep.addAll(partitionIdToEndOfLife.keySet());
+ removePartitionEndOfLifeGauges(realtimeTableName, partitionIdsToKeep);
+ partitionIdToEndOfLife.forEach(
+ (partitionId, endOfLife) ->
setPartitionEndOfLifeGauge(realtimeTableName, partitionId, endOfLife));
Review Comment:
**MINOR — doc vs. behavior.** Every partition that validation repairs
normally hits a `partitionIdToEndOfLife.put(partitionId, false)` path, so this
emits `partitionEndOfLife=0` for all of them. That contradicts the enum comment
("Not emitted for partitions that only go through normal commits"), and it
expands metric cardinality to one series per partition. If emitting 0 broadly
is intended (so alerts can key off it), consider tweaking the enum comment to
match; otherwise only emit when a partition was previously marked ended.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1940,10 +1987,18 @@ && new
LLCSegmentName(segmentEntry.getKey()).getPartitionGroupId() == partitionI
currentTimeMs, instancePartitions, numPartitions,
numReplicas);
updateInstanceStatesForNewConsumingSegment(instanceStatesMap, null,
newSegmentName, segmentAssignment,
instancePartitionsMap);
+ partitionIdToEndOfLife.put(partitionId, false);
}
}
}
+ // Drop the gauges of partitions that no longer have segments, e.g. an
ended partition removed by retention
+ Set<Integer> partitionIdsToKeep = new
HashSet<>(latestSegmentZKMetadataMap.keySet());
+ partitionIdsToKeep.addAll(partitionIdToEndOfLife.keySet());
+ removePartitionEndOfLifeGauges(realtimeTableName, partitionIdsToKeep);
Review Comment:
**MAJOR — concurrency.** This bulk `removeIf` can delete a gauge that a
concurrent commit just set. `commitSegmentMetadata` runs on the completion
thread while this validation run holds an older `latestSegmentZKMetadataMap`
snapshot. If a partition reaches end of life and its last commit sets
`PARTITION_END_OF_LIFE=1` (line 858) after this run snapshotted but before line
1998, that partition isn't in `partitionIdsToKeep`, so `removeIf` deletes the
gauge and drops it from the tracking set — and this run won't re-emit it since
it has no segment for it in view.
The comment above says a concurrent end-of-life mark "is not overwritten",
but that only protects the *value* for partitions already in
`partitionIdToEndOfLife`; a partition absent from this run's snapshot still
gets its gauge removed. Serializing the commit-path mark and this
reconciliation on a per-table lock (or folding the commit mark into validation
so there's a single writer) would close the window. Worth resolving before this
moves out of draft.
--
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]
