Jackie-Jiang commented on code in PR #17215:
URL: https://github.com/apache/pinot/pull/17215#discussion_r2684506474
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -54,6 +55,7 @@
import org.slf4j.LoggerFactory;
import static
org.apache.pinot.spi.utils.CommonConstants.Broker.FALLBACK_POOL_ID;
+import static
org.apache.pinot.spi.utils.CommonConstants.Broker.FALLBACK_REPLICA_GROUP_ID;
Review Comment:
(minor) For conciseness, we can call it `REPLICA_ID`. Replica group is not a
common database term.
We already have mixed usage of `replicaGroup` and `replica`, but going
forward we can probably go with the shorter one
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -245,6 +247,30 @@ static SortedMap<String, String>
convertToSortedMap(Map<String, String> map) {
}
}
+ protected Map<String, Integer> serverToReplicaGroupMap(IdealState
idealState) {
+ Map<String, Integer> serverToReplicaGroupMap = new HashMap<>();
+
+ for (Map.Entry<String, List<String>> listFields :
idealState.getRecord().getListFields().entrySet()) {
+ String key = listFields.getKey();
+ // key looks like "INSTANCE_PARTITIONS__myTable_CONSUMING__0_0"
Review Comment:
+1. Also table name is redundant here
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -245,6 +247,32 @@ static SortedMap<String, String>
convertToSortedMap(Map<String, String> map) {
}
}
+ /// Returns a map from server instance to replica group ID, generated from
the ideal state instance partitions
+ /// metadata.
+ static Map<String, Integer> serverToReplicaGroupMap(IdealState idealState) {
Review Comment:
Should we skip calculating this map when the feature is disabled?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -245,6 +247,32 @@ static SortedMap<String, String>
convertToSortedMap(Map<String, String> map) {
}
}
+ /// Returns a map from server instance to replica group ID, generated from
the ideal state instance partitions
+ /// metadata.
+ static Map<String, Integer> serverToReplicaGroupMap(IdealState idealState) {
+ Map<String, Integer> serverToReplicaGroupMap = new HashMap<>();
+
+ for (Map.Entry<String, List<String>> listFields :
idealState.getRecord().getListFields().entrySet()) {
+ String key = listFields.getKey();
+ // key looks like "INSTANCE_PARTITIONS__myTable_CONSUMING__0_0"
+ if (key.startsWith(InstancePartitionsUtils.IDEAL_STATE_IP_PREFIX)) {
+ String[] parts =
key.split(InstancePartitionsUtils.IDEAL_STATE_IP_SEPARATOR);
Review Comment:
`split` is quite expensive. We can read from right hand side and directly
extract the replica group id
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -270,7 +299,8 @@ void updateSegmentMaps(IdealState idealState, ExternalView
externalView, Set<Str
for (Map.Entry<String, String> entry :
convertToSortedMap(idealStateInstanceStateMap).entrySet()) {
if (isOnlineForRouting(entry.getValue())) {
String instance = entry.getKey();
- candidates.add(new SegmentInstanceCandidate(instance, false,
getPool(instance)));
+ candidates.add(new SegmentInstanceCandidate(instance, false,
getPool(instance),
Review Comment:
For future improvement, consider adding `needPoolId()` and `needReplicaId()`
API to `InstanceSelector` and only fetch them when configured. Doesn't need to
be done in this PR
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -245,6 +247,32 @@ static SortedMap<String, String>
convertToSortedMap(Map<String, String> map) {
}
}
+ /// Returns a map from server instance to replica group ID, generated from
the ideal state instance partitions
+ /// metadata.
+ static Map<String, Integer> serverToReplicaGroupMap(IdealState idealState) {
Review Comment:
(minor) Consider moving this into `InstancePartitionsUtils`
--
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]
