Jackie-Jiang commented on code in PR #8483:
URL: https://github.com/apache/pinot/pull/8483#discussion_r846476628
##########
pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java:
##########
@@ -58,24 +60,39 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String POOLS_KEY = "pools";
+ private static final String REPLICA_GROUP_SEPARATOR = "/";
private final String _instancePartitionsName;
+ // A map to store the partition and its associated list of instances.
+ // The partition key would be like "0_0", where the 1st number denotes the
partition id,
+ // and the 2nd one denotes the replica group id.
private final Map<String, List<String>> _partitionToInstancesMap;
+ // A map to store the selected pool numbers and their associated list of
replica groups.
+ private final Map<Integer, List<Integer>> _poolToReplicaGroupsMap;
Review Comment:
Let's not add this map in this PR for the following reasons:
1. Not mixing the optimization of keeping same server order from the
candidate servers and keeping the same pool for the replica-group (the second
one might not be needed)
2. This map is not general and only apply to a certain algorithm, and might
not apply to cases where replica-group contains servers from multiple pools
3. If we really want to support keeping the same pool for the replica-group
(in a separate PR), we can achieve it by picking the pool that contains the
most common servers with the current assignment. This is more flexible and can
guarantee the minimum movement
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalanceConfigConstants.java:
##########
@@ -33,6 +33,10 @@ private RebalanceConfigConstants() {
public static final String REASSIGN_INSTANCES = "reassignInstances";
public static final boolean DEFAULT_REASSIGN_INSTANCES = false;
+ // Whether to retain the sequence for the existing instances
+ public static final String RETAIN_INSTANCE_SEQUENCE =
"retainInstancesSequence";
Review Comment:
rename it to `minimizeDataMovement`, same for all other places
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/MinimizedDataMovementInstanceReplicaGroupPartitionSelector.java:
##########
@@ -0,0 +1,295 @@
+/**
+ * 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.controller.helix.core.assignment.instance;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.Deque;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.assignment.InstancePartitions;
+import
org.apache.pinot.spi.config.table.assignment.InstanceReplicaGroupPartitionConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * An extended class of {@link InstanceReplicaGroupPartitionSelector} to
minimize data movement between instances.
+ * Currently the following scenarios are supported:
+ * * swap instances within a pool
+ * * add / remove instances per replica group
+ * * increase / decrease number of replica groups
+ *
+ * TODO: Support the remaining scenarios:
+ * * add / remove pools
+ */
+public class MinimizedDataMovementInstanceReplicaGroupPartitionSelector
extends InstanceReplicaGroupPartitionSelector {
+ private static final Logger LOGGER =
+
LoggerFactory.getLogger(MinimizedDataMovementInstanceReplicaGroupPartitionSelector.class);
+
+ private final InstancePartitions _existingInstancePartitions;
+
+ public MinimizedDataMovementInstanceReplicaGroupPartitionSelector(
+ InstanceReplicaGroupPartitionConfig replicaGroupPartitionConfig, String
tableNameWithType,
+ InstancePartitions existingInstancePartitions) {
+ super(replicaGroupPartitionConfig, tableNameWithType);
+ _existingInstancePartitions = existingInstancePartitions;
+ }
+
+ @Override
+ public void selectInstances(Map<Integer, List<InstanceConfig>>
poolToInstanceConfigsMap,
+ InstancePartitions instancePartitions) {
+ int numPools = poolToInstanceConfigsMap.size();
+ Preconditions.checkState(numPools != 0, "No pool qualified for selection");
+
+ int tableNameHash = Math.abs(_tableNameWithType.hashCode());
+ List<Integer> pools = new ArrayList<>(poolToInstanceConfigsMap.keySet());
+ pools.sort(null);
+
Preconditions.checkState(pools.containsAll(_existingInstancePartitions.getPoolToReplicaGroupsMap().keySet()),
Review Comment:
This should be handled instead of throwing exception
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -191,6 +192,27 @@
return instancePartitionsMap;
}
+ /**
+ * Assign instances given the type of instancePartitions.
+ * @param instancePartitionsMap the empty map to be filled.
+ * @param tableConfig table config
+ * @param instanceConfigs list of instance configs
+ * @param instancePartitionsType type of instancePartitions
+ * @param retainInstanceSequence whether to retain instance sequence
+ */
+ private void assignInstancesForInstancePartitionsType(
+ Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap,
TableConfig tableConfig,
+ List<InstanceConfig> instanceConfigs, InstancePartitionsType
instancePartitionsType,
+ boolean retainInstanceSequence) {
Review Comment:
We should set `minimizeDataMovement` into the
`InstanceReplicaGroupPartitionConfig` instead of relying on whether the
existing instance partitions is available
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceReplicaGroupPartitionSelector.java:
##########
@@ -163,31 +163,44 @@ public void selectInstances(Map<Integer,
List<InstanceConfig>> poolToInstanceCon
}
} else {
// Non-replica-group based selection
+ selectForNonReplicaGroupBased(poolToInstanceConfigsMap,
instancePartitions);
+ }
+ }
- // Pick one pool based on the table name hash
- int pool = pools.get(tableNameHash % numPools);
- LOGGER.info("Selecting pool: {} for table: {}", pool,
_tableNameWithType);
- List<InstanceConfig> instanceConfigs =
poolToInstanceConfigsMap.get(pool);
- int numInstanceConfigs = instanceConfigs.size();
-
- // Assign all instances if not configured
- int numInstancesToSelect =
_replicaGroupPartitionConfig.getNumInstances();
- if (numInstancesToSelect > 0) {
- Preconditions.checkState(numInstancesToSelect <= numInstanceConfigs,
- "Not enough qualified instances from pool: %s (%s in the pool,
asked for %s)", pool, numInstanceConfigs,
- numInstancesToSelect);
- } else {
- numInstancesToSelect = numInstanceConfigs;
- }
+ protected void selectForNonReplicaGroupBased(Map<Integer,
List<InstanceConfig>> poolToInstanceConfigsMap,
Review Comment:
For non-replica-group based selection, we can also support minimizing data
movement
--
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: commits-unsubscr...@pinot.apache.org
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
