KKcorps commented on code in PR #19730:
URL: https://github.com/apache/pinot/pull/19730#discussion_r4356808586
##########
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:
Fixed. Every change to a table's gauges now runs inside `compute()` on the
per table map, so the commit mark and the validation update for a table run one
at a time. Each mark carries a sequence number, and a validation run leaves
alone any partition updated after it read the segment metadata, so it can no
longer remove or overwrite a newer mark.
`testPartitionEndOfLifeGaugeMarkedDuringValidation` replays your order and
fails without the fix.
##########
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:
Fixed. Every change to a table's gauges now runs inside `compute()` on the
per table map, so the commit mark and the validation update for a table run one
at a time. Each mark carries a sequence number, and a validation run leaves
alone any partition updated after it read the segment metadata, so it can no
longer remove or overwrite a newer mark.
`testPartitionEndOfLifeGaugeMarkedDuringValidation` replays your order and
fails without the fix.
##########
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:
Fixed. The controller now passes `LeadControllerManager` to the segment
manager, and the commit mark checks leadership inside the same `compute()` that
`removePartitionEndOfLifeGauges` uses. So a mark either lands before the
cleanup and gets removed, or sees that this controller no longer leads the
table. Validation checks it too, because resuming consumption can call it on
any controller. `testPartitionEndOfLifeGaugeSkippedWithoutLeadership` covers it.
##########
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:
Fixed. The controller now passes `LeadControllerManager` to the segment
manager, and the commit mark checks leadership inside the same `compute()` that
`removePartitionEndOfLifeGauges` uses. So a mark either lands before the
cleanup and gets removed, or sees that this controller no longer leads the
table. Validation checks it too, because resuming consumption can call it on
any controller. `testPartitionEndOfLifeGaugeSkippedWithoutLeadership` covers it.
##########
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:
Changed the behavior to match the doc. Only a partition that was marked as
ended before goes back to 0, which is the case alerts need so that a live 0
overrides an older 1. Every other partition emits nothing, so it stays one
series per ended partition, and the enum comment now says exactly that.
`testPartitionEndOfLifeGauge` checks that a new partition emits nothing.
##########
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:
Changed the behavior to match the doc. Only a partition that was marked as
ended before goes back to 0, which is the case alerts need so that a live 0
overrides an older 1. Every other partition emits nothing, so it stays one
series per ended partition, and the enum comment now says exactly that.
`testPartitionEndOfLifeGauge` checks that a new partition emits nothing.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##########
@@ -508,6 +510,23 @@ public void timeoutInactivePartitions() {
_partitionsHostedByThisServer = newMap;
}
+ /// Removes the LLC_PARTITION_CONSUMING gauge of a partition this server no
longer consumes (e.g. an ended Kinesis
+ /// shard), so that it does not stay at 0 forever. The gauge is keyed by
client id, not by partition id.
+ private void removeConsumingGauge(int partitionId) {
+ try {
+ TableConfig tableConfig =
_realTimeTableDataManager.getCachedTableConfigAndSchema().getLeft();
Review Comment:
This code is gone. Offloading the replaced segment already removes this
gauge with the segment's own client id, and the stale 0 came from the `finally`
in `goOnlineFromConsuming` setting it again after that. The `finally` now skips
the set once the segment is offloaded, so nothing rebuilds the key from the
table config anymore.
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTrackerTest.java:
##########
@@ -354,6 +358,34 @@ public void testStopTrackingIngestionDelay() {
}
}
+ @Test
+ public void testTimeoutInactivePartitionsRemovesConsumingGauge() {
Review Comment:
No longer needed, since the tracker does not touch this gauge anymore. The
new server fix uses the segment's own client id, so the partition id encoding
is not involved. `testOnlineTransitionDoesNotAddBackConsumingGaugeAfterReplace`
covers the download and catch up paths, plus a failed replace that keeps the
gauge at 0.
--
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]
