suvodeep-pyne commented on code in PR #19909:
URL: https://github.com/apache/pinot/pull/19909#discussion_r4339384978
##########
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:
Agreed, this holds. `onBecomeOfflineFromError` is a no-op, and
`doOffloadSegment` unregisters the manager before `offload()`, so nothing
guarantees the failed attempt's keys are gone when the reset re-consumes.
Fixed in a779ff97dd. On pauseless dedup and partial-upsert tables, the reset
of an IN_PROGRESS segment with every replica in ERROR is now gated on
`allowRepairOfCommittingSegments`, like re-ingestion. A skipped segment is
logged at WARN, with how to enable the repair, and counted in
`PAUSELESS_SEGMENTS_IN_UNRECOVERABLE_ERROR_COUNT`. That is master's behavior
for this case.
So the only resets this PR adds on those tables are for committed (DONE /
COMMITTING) segments. Their OFFLINE -> ONLINE transition downloads the segment
and never re-consumes. Covered by
`testRepairSegmentsInErrorStateResetsCommittedSegmentsOnPauselessDedupTable`.
The same IN_PROGRESS reset already runs on non-pauseless dedup tables when
`controller.segment.error.autoReset` is on, which is the default. I left that
path as it is on master. Cleaning the dedup / upsert metadata on the server's
ERROR path would fix both, but that is a server change I'd rather make
separately.
##########
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:
Good catch. No, `isAllSegmentsLoaded` doesn't hold it back:
`RealtimeTableDataManager` caches the result once it is true. So it only gates
consumption until all segments are first loaded on that server, typically after
a restart. An ERROR that shows up later doesn't block the consumer, and the
re-consumed rows would merge against values that were never loaded.
In a779ff97dd that reset no longer runs on pauseless partial-upsert or dedup
tables without the override. The segment is skipped with a WARN and counted as
unrecoverable, as on master.
I added
`testRepairSegmentsInErrorStateDoesNotReconsumeAfterErrorSegmentOnPauselessPartialUpsertTable`.
In it, a COMMITTING segment with every replica in ERROR is followed by an
IN_PROGRESS segment with every replica in ERROR. Neither is reset or
re-ingested, and both are counted as unrecoverable.
--
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]
