You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/06 18:28:06 UTC
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #5337: Track unavailable segments in InstanceSelector
jackjlli commented on a change in pull request #5337:
URL: https://github.com/apache/incubator-pinot/pull/5337#discussion_r420999890
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
##########
@@ -85,94 +88,147 @@ public void onInstancesChange(Set<String> enabledInstances, List<String> changed
return;
}
- // Update the map from segment to enabled instances
+ // Update the map from segment to enabled ONLINE/CONSUMING instances and set of unavailable segments (no enabled
+ // instance or all enabled instances are in ERROR state)
// NOTE: We can directly modify the map because we will only update the values without changing the map entries.
// Because the map is marked as volatile, the running queries (already accessed the map) might use the enabled
// instances either before or after the change, which is okay; the following queries (not yet accessed the map) will
// get the updated value.
- Map<String, List<String>> segmentToInstancesMap = _segmentToInstancesMap;
+ Map<String, List<String>> segmentToOnlineInstancesMap = _segmentToOnlineInstancesMap;
+ Map<String, List<String>> segmentToOfflineInstancesMap = _segmentToOfflineInstancesMap;
Map<String, List<String>> segmentToEnabledInstancesMap = _segmentToEnabledInstancesMap;
+ Set<String> currentUnavailableSegments = _unavailableSegments;
Review comment:
It should be fine, it's just a reference.
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
##########
@@ -85,94 +88,147 @@ public void onInstancesChange(Set<String> enabledInstances, List<String> changed
return;
}
- // Update the map from segment to enabled instances
+ // Update the map from segment to enabled ONLINE/CONSUMING instances and set of unavailable segments (no enabled
+ // instance or all enabled instances are in ERROR state)
// NOTE: We can directly modify the map because we will only update the values without changing the map entries.
// Because the map is marked as volatile, the running queries (already accessed the map) might use the enabled
// instances either before or after the change, which is okay; the following queries (not yet accessed the map) will
// get the updated value.
- Map<String, List<String>> segmentToInstancesMap = _segmentToInstancesMap;
+ Map<String, List<String>> segmentToOnlineInstancesMap = _segmentToOnlineInstancesMap;
+ Map<String, List<String>> segmentToOfflineInstancesMap = _segmentToOfflineInstancesMap;
Map<String, List<String>> segmentToEnabledInstancesMap = _segmentToEnabledInstancesMap;
+ Set<String> currentUnavailableSegments = _unavailableSegments;
+ Set<String> newUnavailableSegments = new HashSet<>();
for (Map.Entry<String, List<String>> entry : segmentToEnabledInstancesMap.entrySet()) {
String segment = entry.getKey();
if (segmentsToUpdate.contains(segment)) {
- entry.setValue(
- calculateEnabledInstancesForSegment(segment, segmentToInstancesMap.get(segment), enabledInstances));
+ List<String> enabledInstancesForSegment =
+ calculateEnabledInstancesForSegment(segment, segmentToOnlineInstancesMap.get(segment),
+ segmentToOfflineInstancesMap, enabledInstances, newUnavailableSegments);
+ entry.setValue(enabledInstancesForSegment);
+ } else {
+ if (currentUnavailableSegments.contains(segment)) {
+ newUnavailableSegments.add(segment);
+ }
}
}
+ _unavailableSegments = newUnavailableSegments;
}
@Override
public void onExternalViewChange(ExternalView externalView, Set<String> onlineSegments) {
Review comment:
It might be good to put some comments on what you're gonna do in this method, as the logic becomes complex.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org