suvodeep-pyne commented on code in PR #19909:
URL: https://github.com/apache/pinot/pull/19909#discussion_r4339385988
##########
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:
Intentional, and unchanged from master on every table: master's case 3 also
excluded IN_PROGRESS. Resetting that replica would re-consume the segment on
it, which is the problem from your first comment on dedup and partial-upsert
tables. Waiting for the commit means the reset then downloads the committed
segment. With `enforceConsumptionInOrder`, that server's next segment of the
partition waits until then, as on master.
In a779ff97dd the case has its own branch, with an INFO log saying the
replicas will be reset once the segment commits. The branch also covers a
COMMITTING segment whose commit hasn't updated the IdealState yet, since a
reset would re-consume that one too. It is not counted as unrecoverable, since
it heals on its own, unless no replica is consuming it at all, for example when
the others are OFFLINE. Then it is counted. Since
`PAUSELESS_SEGMENTS_IN_ERROR_COUNT` is back to counting segments with every
replica in ERROR (see your metric comment), it no longer counts this case
either. The ERROR replica is in `SEGMENTS_IN_ERROR_STATE` from
`SegmentStatusChecker`.
On collapsing cases 2 and 3: a reset re-consumes an IN_PROGRESS segment, so
I kept "every replica in ERROR" as the point where re-consuming is the only way
forward.
##########
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) {
Review Comment:
Agreed; reverted in a779ff97dd. `PAUSELESS_SEGMENTS_IN_ERROR_COUNT`, and
`SEGMENTS_IN_ERROR_STATE` as written by this method, count segments with every
replica in ERROR again, as on master.
I didn't add a new gauge. A single ERROR replica is already visible through
`SEGMENTS_IN_ERROR_STATE`, which `SegmentStatusChecker` writes for every table,
pauseless included, as the number of (segment, instance) ERROR pairs. The only
change left for this gauge is that dedup and partial-upsert tables now set it
at all; on master the early return skipped it.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3036,6 +3036,24 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
}
}
+ /// Resets only the replicas of the segment that are in ERROR state in the
external view. Healthy replicas are left
+ /// untouched: resetting a healthy replica takes it through OFFLINE, which
on dedup / upsert tables briefly drops its
+ /// keys from the metadata manager while other replicas keep consuming, and
can make the only good copy of a
+ /// segment without a deep-store copy unavailable.
+ private void resetErrorReplicas(String realtimeTableName, String
segmentName, Map<String, String> currentStateMap) {
+ for (Map.Entry<String, String> entry : currentStateMap.entrySet()) {
+ if (SegmentStateModel.ERROR.equals(entry.getValue())) {
+ String instanceName = entry.getKey();
+ try {
Review Comment:
Agreed. `resetErrorReplicas` (added in a779ff97dd) checks each ERROR replica
against the IdealState it reads. When it skips one, it logs a WARN naming the
segment, table and instance. Covered by
`testResetErrorReplicasResetsOnlyReplicasStillInError` (`instance_3`).
--
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]
