suvodeep-pyne commented on code in PR #19909:
URL: https://github.com/apache/pinot/pull/19909#discussion_r4339388291
##########
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.",
Review Comment:
This is a real server-side gap. But after the changes in a779ff97dd, this PR
no longer issues an OFFLINE -> CONSUMING reset on any table where master didn't:
- On pauseless dedup and partial-upsert tables, the IN_PROGRESS reset is
gated again.
- A COMMITTING segment that is still CONSUMING in the IdealState (its commit
hasn't updated the IdealState yet) is now handled like an IN_PROGRESS one: its
ERROR replicas wait for the commit instead of being reset into CONSUMING.
- The resets that remain take the OFFLINE -> ONLINE path, which already
handles a stale registered manager (the lines you point to).
- On every other table, the IN_PROGRESS reset already runs on master.
So I'd fix `doAddConsumingSegment` in a separate server PR rather than here.
Does that work for you?
##########
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) {
Review Comment:
Agreed on batching; done in a779ff97dd. `resetErrorReplicas` reads the
IdealState and ExternalView once per pass and sends one reset batch per
instance. A pass now costs one IdealState and ExternalView read plus one
batched reset per instance. Before, `resetSegment` re-read both for every
replica it reset.
Master wasn't O(segments) either: `resetPartitionAllState` reads the full
IdealState for every replica it resets. That read is now per instance.
I didn't reuse the IdealState and ExternalView from the top of the method,
because your race comment needs fresh state.
To keep one segment from blocking the batch, the error-only reset skips a
segment that already has a pending state transition message (for example, the
previous pass's reset) instead of failing the whole instance, as
`resetSegments` does.
--
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]
