xiangfu0 commented on code in PR #20157:
URL: https://github.com/apache/pinot/pull/20157#discussion_r4475104008
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3285,6 +3287,18 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
continue;
}
+ // A reset either re-consumes the segment or reloads it. On a pauseless
dedup or partial upsert table, the server
+ // may have consumed the later segments without this one's keys, so
neither is safe there without the override.
+ if (isPauselessTable && !repairCommittingSegments.get()) {
Review Comment:
This needs an owner's decision rather than a code fix. The gate also skips
the reset when segment N failed *before* the server's consume gate latched
(`isAllSegmentsLoaded` returns false on any ERROR). In that case reloading is
safe, and it is the only thing that lets that server ingest any partition of
the table again; that is the case #19909 fixed.
If owners agree this is the safe default, please have the release notes or
runbook point operators to `PAUSELESS_SEGMENTS_IN_UNRECOVERABLE_ERROR_COUNT`
and the two opt-ins.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3285,6 +3287,18 @@ public void repairSegmentsInErrorState(TableConfig
tableConfig,
continue;
}
+ // A reset either re-consumes the segment or reloads it. On a pauseless
dedup or partial upsert table, the server
+ // may have consumed the later segments without this one's keys, so
neither is safe there without the override.
+ if (isPauselessTable && !repairCommittingSegments.get()) {
+ unrecoverableSegments.add(segmentName);
Review Comment:
Heads-up on what this gauge counts. In `release-1.6.0` it was
`segmentsInErrorStateInAllReplicas.size()`. Now it counts every segment with at
least one ERROR replica, including IN_PROGRESS segments still CONSUMING on a
healthy replica.
That is arguably the right meaning ("won't recover on its own"), but alerts
tuned on 1.6.0 will start firing when a single replica fails. Worth a release
note.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -3141,8 +3141,10 @@ URI createSegmentPath(String rawTableName, String
segmentName) {
/// Consuming from the stream again is unsafe on dedup and partial upsert
tables: re-ingestion can admit duplicate
/// or inconsistent rows, and a re-consumed segment can drop rows as
duplicates of keys left by the failed attempt,
/// or merge partial upsert rows against previous values that no replica has
loaded. So unless
- /// [#allowRepairOfCommittingSegments] allows it, such tables skip
re-ingestion and, when pauseless, the reset that
- /// re-consumes an IN_PROGRESS segment. Resets that reload a segment run for
every table.
+ /// [#allowRepairOfCommittingSegments] allows it, such tables skip
re-ingestion and, when pauseless, every reset.
+ /// Reloading a segment is unsafe on them too: a pauseless server can keep
consuming the segments after a failed one
+ /// without the failed segment's keys, so reloading it can put a replica
that holds wrong rows back into routing.
+ /// Non-pauseless tables still reset the replicas of segments that reload.
Review Comment:
Nit: non-pauseless servers have the same hazard; the PR description lists it
as a known gap under `controller.segment.error.autoReset`. Read together, "a
pauseless server can keep consuming…" and "Non-pauseless tables still reset…"
suggest their reloads are safe. Maybe say instead that non-pauseless tables
aren't gated here and remain a known gap.
--
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]
