xiangfu0 commented on code in PR #18731:
URL: https://github.com/apache/pinot/pull/18731#discussion_r3395695007
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -222,10 +227,28 @@ void updateSegmentMapsForUpsertTable(IdealState
idealState, ExternalView externa
// Calculate the unavailable instances based on the old segments' online
instances for each combination of instances
// in the ideal state
Map<Set<String>, Set<String>> unavailableInstancesMap = new HashMap<>();
+ List<String> segmentsNotOnlineAnywhere = null;
for (Map.Entry<String, Set<String>> entry :
oldSegmentToOnlineInstancesMap.entrySet()) {
String segment = entry.getKey();
Set<String> onlineInstances = entry.getValue();
Map<String, String> idealStateInstanceStateMap =
idealStateAssignment.get(segment);
+ // When a segment is not online on any instance in the ideal state (e.g.
it was just moved to a new set of
+ // instances by tier relocation/rebalance and is still being loaded on
all of them), no instance selection can
+ // serve it, so excluding instances for the whole group cannot help
consistency. Marking all its instances
+ // unavailable would instead take down every other segment sharing the
same instances (a production incident
+ // showed a whole tier of ~30k segments reported unavailable due to a
few relocating segments). Skip it here;
+ // it gets an empty candidate list below and is reported unavailable
individually.
+ if (onlineInstances.isEmpty()) {
Review Comment:
This shared helper is also the path that
`ReplicaGroupInstanceSelector.updateSegmentMaps()` uses for upsert/dedup
tables. Skipping the group-wide exclusion here changes those tables from
"partition unavailable" to "serve older rows": if the latest segment for a PK
is the nowhere-online segment, we now route to an older segment in the same
group and surface a superseded value. That is a correctness regression, not
just an availability tradeoff. This exception likely needs to stay out of the
upsert/dedup path (or be gated separately) instead of applying in the shared
helper.
--
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]
