yupeng9 commented on a change in pull request #6208:
URL: https://github.com/apache/incubator-pinot/pull/6208#discussion_r513870000
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
##########
@@ -126,47 +128,28 @@ public void onInstancesChange(Set<String>
enabledInstances, List<String> changed
* {@inheritDoc}
*
* <p>Updates the cached maps ({@code segmentToOnlineInstancesMap}, {@code
segmentToOfflineInstancesMap} and
- * {@code instanceToSegmentsMap}) based on the given ExternalView and
re-calculates
- * {@code segmentToEnabledInstancesMap} and {@code unavailableSegments}
based on the cached states.
+ * {@code instanceToSegmentsMap}) and re-calculates {@code
segmentToEnabledInstancesMap} and
+ * {@code unavailableSegments} based on the cached states.
*/
@Override
- public void onExternalViewChange(ExternalView externalView, Set<String>
onlineSegments) {
- Map<String, Map<String, String>> segmentAssignment =
externalView.getRecord().getMapFields();
- int numSegments = segmentAssignment.size();
- _segmentToOnlineInstancesMap = new
HashMap<>(HashUtil.getHashMapCapacity(numSegments));
- _segmentToOfflineInstancesMap = new
HashMap<>(HashUtil.getHashMapCapacity(numSegments));
+ public void onExternalViewChange(ExternalView externalView, IdealState
idealState, Set<String> onlineSegments) {
Review comment:
`onlineSegments` can be derived from `idealState `?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/RoutingManager.java
##########
@@ -284,6 +278,7 @@ private static boolean isInstanceEnabled(ZNRecord
instanceConfigZNRecord) {
if
("true".equals(instanceConfigZNRecord.getSimpleField(CommonConstants.Helix.IS_SHUTDOWN_IN_PROGRESS)))
{
return false;
}
+ //noinspection RedundantIfStatement
Review comment:
noinspection -> No inspection
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/StrictReplicaGroupInstanceSelector.java
##########
@@ -0,0 +1,179 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.routing.instanceselector;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import
org.apache.pinot.common.utils.CommonConstants.Helix.StateModel.SegmentStateModel;
+import org.apache.pinot.common.utils.HashUtil;
+
+
+/**
+ * Instance selector for strict replica-group routing strategy.
+ *
+ * <pre>
+ * The strict replica-group routing strategy always routes the query to the
instances within the same replica-group.
+ * (Note that the replica-group information is derived from the ideal state of
the table, where the instances are sorted
+ * alphabetically in the instance state map, so the replica-groups in the
instance selector might not match the
+ * replica-groups in the instance partitions.) The instances in a
replica-group should have all the online segments
+ * (segments with ONLINE/CONSUMING instances in the ideal state and selected
by the pre-selector) available
+ * (ONLINE/CONSUMING in the external view) in order to serve queries. If any
segment is unavailable in the
+ * replica-group, we mark the whole replica-group down and not serve queries
with this replica-group.
+ *
+ * The selection algorithm is the same as {@link
ReplicaGroupInstanceSelector}, and will always evenly distribute the
+ * traffic to all replica-groups that have all online segments available.
+ *
+ * The algorithm relies on the mirror segment assignment from replica-group
segment assignment strategy. With mirror
+ * segment assignment, any server in one replica-group will always have a
corresponding server in other replica-groups
+ * that have the same segments assigned. For example, if S1 is a server in
replica-group 1, and it has mirror server
+ * S2 in replica-group 2 and S3 in replica-group 3. All segments assigned to
S1 will also be assigned to S2 and S3. In
+ * stable scenario (external view matches ideal state), all segments assigned
to S1 will have the same enabled instances
+ * of [S1, S2, S3] sorted (in alphabetical order). If we always pick the same
index of enabled instances for all
+ * segments, only one of S1, S2, S3 will be picked, and all the segments are
processed by the same server. In
+ * transitioning/error scenario (external view does not match ideal state), if
a segment is down on S1, we mark all
+ * segments with the same assignment ([S1, S2, S3]) down on S1 to ensure that
we always route the segments to the same
+ * replica-group.
+ * </pre>
+ */
+public class StrictReplicaGroupInstanceSelector extends
ReplicaGroupInstanceSelector {
+
+ public StrictReplicaGroupInstanceSelector(String tableNameWithType,
BrokerMetrics brokerMetrics) {
+ super(tableNameWithType, brokerMetrics);
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * <pre>
+ * The maps are calculated in the following steps to meet the strict
replica-group guarantee:
+ * 1. Create a map from online segment to set of instances hosting the
segment based on the ideal state
+ * 2. Gather the online and offline instances for each online segment from
the external view
+ * 3. Compare the instances from the ideal state and the external view and
gather the unavailable instances for each
+ * set of instances
+ * 4. Exclude the unavailable instances from the online instances map
+ * </pre>
+ */
+ @Override
+ void updateSegmentMaps(ExternalView externalView, IdealState idealState,
Set<String> onlineSegments,
Review comment:
high-level comment, since this is to calculate the map based on the
coarse-grained unit (i.e. replica group) instead of the segment, have you
considered mapping the online segments to the replica groups first, and then
use the groups for selection?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
