raghavyadav01 commented on code in PR #19909:
URL: https://github.com/apache/pinot/pull/19909#discussion_r4321200625
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3020,13 +3020,13 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
if (status == Status.IN_PROGRESS &&
segmentsInErrorStateInAllReplicas.contains(segmentName)) {
LOGGER.info("Resetting IN_PROGRESS segment: {} in table: {} since all
replicas are in ERROR state.",
segmentName, realtimeTableName);
- _helixResourceManager.resetSegment(realtimeTableName, segmentName,
null);
+ resetErrorReplicas(realtimeTableName, segmentName, currentStateMap);
Review Comment:
On a partial-upsert table this reset now re-consumes the IN_PROGRESS segment
from its start offset. If an earlier COMMITTING segment of the same partition
is still in ERROR (re-ingestion gated off), the re-consumed rows would merge
against missing previous values. Is `TableStateUtils.isAllSegmentsLoaded` what
holds the consumer back in that case? Could we add a test where the previous
segment stays in ERROR?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2956,22 +2958,14 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
boolean repairCommittingSegments =
allowRepairOfCommittingSegments(repairErrorSegmentsForPartialUpsertOrDedup,
tableConfig);
int segmentsInUnRecoverableState = 0;
- if (isPauselessTable && !repairCommittingSegments) {
- // We do not run reingestion for dedup and partial upsert tables in
pauseless as it can
- // lead to data inconsistencies
- _controllerMetrics.setOrUpdateTableGauge(realtimeTableName,
- ControllerGauge.PAUSELESS_SEGMENTS_IN_UNRECOVERABLE_ERROR_COUNT,
segmentsInErrorStateInAllReplicas.size());
- return;
- } else {
- LOGGER.info("Repairing error segments in table: {}.", realtimeTableName);
- }
+ // Count segments with any ERROR replica so a single lagging replica is
visible in the metric
if (isPauselessTable) {
_controllerMetrics.setOrUpdateTableGauge(realtimeTableName,
ControllerGauge.PAUSELESS_SEGMENTS_IN_ERROR_COUNT,
- segmentsInErrorStateInAllReplicas.size());
+ segmentsInErrorStateInAtLeastOneReplica.size());
Review Comment:
This changes what `PAUSELESS_SEGMENTS_IN_ERROR_COUNT` and
`segmentsInErrorState` mean (any ERROR replica instead of all replicas). Alerts
tuned to the old meaning will fire more often. Worth an upgrade note in the PR
description?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3020,13 +3020,13 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
if (status == Status.IN_PROGRESS &&
segmentsInErrorStateInAllReplicas.contains(segmentName)) {
LOGGER.info("Resetting IN_PROGRESS segment: {} in table: {} since all
replicas are in ERROR state.",
segmentName, realtimeTableName);
- _helixResourceManager.resetSegment(realtimeTableName, segmentName,
null);
+ resetErrorReplicas(realtimeTableName, segmentName, currentStateMap);
continue;
}
// 3) For all other non-IN_PROGRESS statuses with any ERROR replica,
trigger a reset to download/refresh
if (status != Status.IN_PROGRESS) {
Review Comment:
An IN_PROGRESS segment with only some replicas in ERROR skips both branches,
so it isn't repaired and isn't counted in
`PAUSELESS_SEGMENTS_IN_UNRECOVERABLE_ERROR_COUNT`. It heals after commit, but
the description says the gauge counts everything this pass leaves unrepaired.
Should it count these too, or should the description say so?
--
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]
