[GitHub] [helix] jiajunwang commented on issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer
jiajunwang commented on issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer URL: https://github.com/apache/helix/issues/342#issuecomment-517893119 The foundation of the new rebalancer has been built after the interface/cluster model checked in. I'm closing this issue since we will have separate issues to track the following components development. 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang closed issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer
jiajunwang closed issue #342: Kickoff for the Weight Aware Globally Even Distribute Rebalancer URL: https://github.com/apache/helix/issues/342 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang merged pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang merged pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517892944 This PR is ready to be merged, approved by @narendly. I will do the merge. 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310338335
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
The rebalance logic will take care of the resource, partition, and also
state priority.
The priority we recorded in the replica is mainly for the top state. Since
that is really something special.
In fact, I tried to use a boolean isTopState. But then I find the number of
priority already exists and it is the most direct information. The model class
shall not do extra calculation.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336821
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,121 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
+ /**
+ * @param resourceConfig The resource config for the resource which contains
the replication.
+ * @param partitionName The replication's partition name.
+ * @param replicaState The state of the replication.
+ * @param statePriority The priority of the replication's state.
+ */
+ AssignableReplica(ResourceConfig resourceConfig, String partitionName,
String replicaState,
+ int statePriority) {
+_partitionName = partitionName;
+_replicaState = replicaState;
+_statePriority = statePriority;
+_resourceName = resourceConfig.getResourceName();
+_capacityUsage = fetchCapacityUsage(partitionName, resourceConfig);
Review comment:
I would only advocate for the Builder pattern if there were more arguments
(or if we expect to add more arguments to the constructor). To me, this is OK.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336903
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
Review comment:
I like the descriptive comments. Thanks :)
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336872
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void assign(String resourceName, String partitionName, String state,
String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.assign(replica);
+_clusterContext.addPartitionToFaultZone(node.getFaultZone(), resourceName,
partitionName);
+ }
+
+ /**
+ * Revert the proposed assignment from the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void release(String resourceName, String partitionName, String state,
+ String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.release(replica);
+_clusterContext.removePartitionFromFaultZone(node.getFaultZone(),
resourceName, partitionName);
+ }
+
+ private AssignableNode locateAssignableNode(String instanceName) {
+AssignableNode node = _assignableNodeMap.get(instanceName);
+if (node == null) {
+ throw new HelixException("Cannot find the instance: " + instanceName);
Review comment:
Sorry I don't think I understand.
This is a private method so I don't think anyone is going to call it but us,
but what would it mean when an AssignableNode is not found for the given
instanceName? Would that be an indication that we messed up somewhere?
This is an automated message from the Apache Git
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336640
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
I think I was thinking about priority among different partitions. I don't
know how the assignment logic works (is it part of this PR or is it TBD in
another PR?), but I agree with you if we assign replicas by partition.
I was just bringing up the fact that we may need to account partition-level
or resource-level priority at assign time. OK to resolve this issue since we
could always revisit later.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336694
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,287 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger LOG =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition; // maximum number of the partitions that can be
assigned to the node.
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // The maximum capacity utilization (0.0 - 1.0) across all the capacity
categories.
+ private float _highestCapacityUtilization;
+
+ AssignableNode(ClusterConfig clusterConfig, InstanceConfig instanceConfig,
String instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterConfig, instanceConfig, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculates currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to change. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterConfig - the Cluster Config of the cluster where the node
is located
+ * @param instanceConfig - the Instance Config of the node
+ * @param existingAssignment - all the existing replicas that are current
assigned to the node
+ */
+ private void refresh(ClusterConfig clusterConfig, InstanceConfig
instanceConfig,
+ Collection existingAssignment) {
+reset();
+
+_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+_faultZone = computeFaultZone(clusterConfig, instanceConfig);
+_instanceTags = new HashSet<>(instanceConfig.getTags());
+_disabledPartitionsMap = instanceConfig.getDisabledPartitionsMap();
+_maxCapacity = instanceConfig.getInstanceCapacityMap();
+_maxPartition = clusterConfig.getMaxPartitionsPerInstance();
+
+assignNewBatch(existingAssignment);
+ }
+
+ /**
+ * Assign a replica to the node.
+ *
+ * @param assignableReplica - the replica to be assigned
+ */
+ void assign(AssignableReplica assignableReplica) {
+if (!addToAssignmentRecord(assignableReplica, _currentAssignments)) {
+ throw new HelixException(String
+ .format("Resource %s already has a replica from partition %s on node
%s",
+ assignableReplica.getResourceName(),
assignableReplica.getPartitionName(),
+ getInstanceName()));
+} else {
+ if (assignableReplica.isReplicaTopState()) {
+addToAssignmentRecord(assignableReplica, _currentTopStateAssignments);
+ }
+ assignableReplica.getCapacity().entrySet().stream().forEach(
+ capacity -> updateCapacityAndUtilization(capacity.getKey(),
capacity.getValue()));
+}
+ }
+
+ /**
+ * Release a replica from the node.
+ * If the replication is not on this node, the assignable node is not
updated.
+ *
+ * @param assignableReplica - the replica to be released
+ */
+ void release(AssignableReplica assignableReplica) throws
IllegalArgumentException {
+String resourceName = assignableReplica.getResourceName();
+String partitionName = assignableReplica.getPartitionName();
+
+// Check if the release is necessary
+if
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336640
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,119 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
Review comment:
I think I was thinking about priority among different partitions. I don't
know how the assignment logic works (is it part of this PR or is it TBD in
another PR?), but I agree with you if we assign replicas by partition.
I was just bringing up the fact that we may need to account partition-level
or resource-level priority at assign time.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310336585
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,293 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger _logger =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition;
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // runtime usage tracking
+ private int _totalReplicaAssignmentCount;
+ private float _highestCapacityUtilization;
+
+ AssignableNode(ResourceControllerDataProvider clusterCache, String
instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterCache, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_totalReplicaAssignmentCount = 0;
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculate currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to changes. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterCache - the current cluster cache to initial the
AssignableNode.
+ */
+ private void refresh(ResourceControllerDataProvider clusterCache,
+ Collection existingAssignment) {
+reset();
+
+InstanceConfig instanceConfig =
clusterCache.getInstanceConfigMap().get(_instanceName);
+ClusterConfig clusterConfig = clusterCache.getClusterConfig();
+
+_currentCapacity.putAll(instanceConfig.getInstanceCapacityMap());
+_faultZone = computeFaultZone(clusterConfig, instanceConfig);
+_instanceTags = new HashSet<>(instanceConfig.getTags());
+_disabledPartitionsMap = instanceConfig.getDisabledPartitionsMap();
+_maxCapacity = instanceConfig.getInstanceCapacityMap();
+_maxPartition = clusterConfig.getMaxPartitionsPerInstance();
+
+assignNewBatch(existingAssignment);
+ }
+
+ /**
+ * Assign a replica to the node.
+ *
+ * @param assignableReplica - the replica to be assigned
+ */
+ void assign(AssignableReplica assignableReplica) {
+if (!addToAssignmentRecord(assignableReplica, _currentAssignments)) {
+ throw new HelixException(String
+ .format("Resource %s already has a replica from partition %s on this
node",
+ assignableReplica.getResourceName(),
assignableReplica.getPartitionName()));
+} else {
+ if (assignableReplica.isReplicaTopState()) {
+addToAssignmentRecord(assignableReplica, _currentTopStateAssignments);
+ }
+ _totalReplicaAssignmentCount += 1;
+ assignableReplica.getCapacity().entrySet().stream()
+ .forEach(entry -> updateCapacityAndUtilization(entry.getKey(),
entry.getValue()));
+}
+ }
+
+ /**
+ * Release a replica from the node.
+ * If the replication is not on this node, the assignable node is not
updated.
+ *
+ * @param assignableReplica - the replica to be released
+ */
+ void release(AssignableReplica assignableReplica) throws
IllegalArgumentException {
+String resourceName = assignableReplica.getResourceName();
+String partitionName = assignableReplica.getPartitionName();
+
+// Check if the release is necessary
+if
[GitHub] [helix] jiajunwang opened a new issue #372: Implement the Cluster Model data provider for the WAGED rebalancer
jiajunwang opened a new issue #372: Implement the Cluster Model data provider for the WAGED rebalancer URL: https://github.com/apache/helix/issues/372 Please find the design doc here: https://github.com/apache/helix/wiki/Design-Proposal---Weight-Aware-Globally-Even-Distribute-Rebalancer This data provider will help to convert Helix cluster status data into the rebalancer readable Cluster Model object. The result will be used to support rebalance calculation. AC: - finish the implementation based on the design. - add unit tests for the new data provider. 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517854427 Thanks for your hard work of reviewing. I know it is a huge PR. Mainly because it is new classes that are built from scratch, even one file's change has been large enough. And as the initial interface checking, I wish to keep then in one PR. The following functional components will still be a large file, but they will be loosely coupled. So we will be able to make relatively smaller PR. 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310284240
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableNode.java
##
@@ -19,10 +19,290 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static java.lang.Math.max;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class represents a potential allocation of the replication.
- * Note that AssignableNode is not thread safe.
+ * This class represents a possible allocation of the replication.
+ * Note that any usage updates to the AssignableNode are not thread safe.
*/
-public class AssignableNode { }
+public class AssignableNode {
+ private static final Logger _logger =
LoggerFactory.getLogger(AssignableNode.class.getName());
+
+ // proposed assignment tracking
+ //
+ private Map> _currentAssignments;
+ //
+ private Map> _currentTopStateAssignments;
+ //
+ private Map _currentCapacity;
+ // runtime usage tracking
+ private int _totalReplicaAssignmentCount;
+ private float _highestCapacityUtilization;
+
+ // basic node information
+ private final String _instanceName;
+ private Set _instanceTags;
+ private String _faultZone;
+ private Map> _disabledPartitionsMap;
+ private Map _maxCapacity;
+ private int _maxPartition;
+
+ AssignableNode(ResourceControllerDataProvider clusterCache, String
instanceName,
+ Collection existingAssignment) {
+_instanceName = instanceName;
+refresh(clusterCache, existingAssignment);
+ }
+
+ private void reset() {
+_currentAssignments = new HashMap<>();
+_currentTopStateAssignments = new HashMap<>();
+_currentCapacity = new HashMap<>();
+_totalReplicaAssignmentCount = 0;
+_highestCapacityUtilization = 0;
+ }
+
+ /**
+ * Update the node with a ClusterDataCache. This resets the current
assignment and recalculate currentCapacity.
+ * NOTE: While this is required to be used in the constructor, this can also
be used when the clusterCache needs to be
+ * refreshed. This is under the assumption that the capacity mappings of
InstanceConfig and ResourceConfig could
+ * subject to changes. If the assumption is no longer true, this function
should become private.
+ *
+ * @param clusterCache - the current cluster cache to initial the
AssignableNode.
+ */
+ private void refresh(ResourceControllerDataProvider clusterCache,
Review comment:
Some background here. We want to have a cluster data cache snapshot based on
DataProvider. And that should have been used here. The snapshot is immutable.
Unfortunately, we don't have this class implemented yet.
This provider the closest thing we can use for now. We will need to refactor
the usage of the data provider everywhere once the snapshot is done. This is
planed in the scope of controller improvement.
About the second point, the data provider does not know anything about data
model. This is the current situation. I guess you are saying that data model
shall not rely on the data provider's methods, right?
This is a valid call, let me try to change the refresh method parameters.
But still, a builder is too complicated for now.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310279995
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
Review comment:
This is a typo. Good catch! Should check the next line.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310280122
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
+.remove(_partitionNames.get(0)));
+context.removePartitionFromFaultZone(_testFaultZoneId,
_resourceNames.get(0),
+_partitionNames.get(0));
+
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+ }
+
+ @Test(expectedExceptions = HelixException.class,
expectedExceptionsMessageRegExp = "Resource Resource1 already has a replica
from partition Partition1 in fault zone testZone")
+ public void testAssignAlreadyExist() throws IOException {
Review comment:
testDuplicateAssign
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310278490
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
+Map capacityDataMapResource1 = new HashMap<>();
+capacityDataMapResource1.put("item1", 3);
+capacityDataMapResource1.put("item2", 6);
+ResourceConfig testResourceConfigResource = new
ResourceConfig(resourceName);
+testResourceConfigResource.setPartitionCapacityMap(
+Collections.singletonMap(ResourceConfig.DEFAULT_PARTITION_KEY,
capacityDataMapResource1));
+
+String partitionName = partitionNamePrefix + 1;
+AssignableReplica replica =
+new AssignableReplica(testResourceConfigResource, partitionName,
masterState,
+masterPriority);
+Assert.assertEquals(replica.getResourceName(), resourceName);
Review comment:
In that case, we will still need to list all the different parameters.
Moreover, if anything new to check in different places, we need to split the
method or check redundantly. So I prefer to keep the Asserts listed in the test
case.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#discussion_r310277680 ## File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java ## @@ -0,0 +1,100 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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. + */ + +import org.apache.helix.model.ResourceConfig; +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY; Review comment: This is automatically done... I'm fine with both. Will change it. 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 With regards, Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310277478
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310277080
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Missing 0.9.0.1 Helix release in maven repository
Hi Folks, We are generating an Apache Helix release 0.9.0.1. Everything looks good after we click the release in Repository Manager. But we did not find the 0.9.0.1 release in maven repository until now. Could you please help us figure it out why it is not showing up? It was released yesterday 6pm. Could that be maven only supports 3 level version? Best, Junkai
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310276844
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310249111
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
Review comment:
Sure, make sense.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310248906
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void assign(String resourceName, String partitionName, String state,
String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.assign(replica);
+_clusterContext.addPartitionToFaultZone(node.getFaultZone(), resourceName,
partitionName);
+ }
+
+ /**
+ * Revert the proposed assignment from the cluster model.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param instanceName
+ */
+ public void release(String resourceName, String partitionName, String state,
+ String instanceName) {
+AssignableNode node = locateAssignableNode(instanceName);
+AssignableReplica replica = locateAssignableReplica(resourceName,
partitionName, state);
+
+node.release(replica);
+_clusterContext.removePartitionFromFaultZone(node.getFaultZone(),
resourceName, partitionName);
+ }
+
+ private AssignableNode locateAssignableNode(String instanceName) {
+AssignableNode node = _assignableNodeMap.get(instanceName);
+if (node == null) {
+ throw new HelixException("Cannot find the instance: " + instanceName);
Review comment:
This is a public interface. If anyone calls it in a wrong way, we'd better
have a HelixException instead of NPE.
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,
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310246676
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
Review comment:
Good catch
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310245795
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/AssignableReplica.java
##
@@ -19,9 +19,121 @@
* under the License.
*/
+import org.apache.helix.model.ResourceConfig;
+import org.apache.helix.model.StateModelDefinition;
+
+import java.io.IOException;
+import java.util.Map;
+
/**
- * A placeholder before we have the implementation.
- *
* This class represents a partition replication that needs to be allocated.
*/
-public class AssignableReplica { }
+public class AssignableReplica implements Comparable {
+ private final String _partitionName;
+ private final String _resourceName;
+ private final String _resourceInstanceGroupTag;
+ private final int _resourceMaxPartitionsPerInstance;
+ private final Map _capacityUsage;
+ // The priority of the replica's state
+ private final int _statePriority;
+ // The state of the replica
+ private final String _replicaState;
+
+ /**
+ * @param resourceConfig The resource config for the resource which contains
the replication.
+ * @param partitionName The replication's partition name.
+ * @param replicaState The state of the replication.
+ * @param statePriority The priority of the replication's state.
+ */
+ AssignableReplica(ResourceConfig resourceConfig, String partitionName,
String replicaState,
+ int statePriority) {
+_partitionName = partitionName;
+_replicaState = replicaState;
+_statePriority = statePriority;
+_resourceName = resourceConfig.getResourceName();
+_capacityUsage = fetchCapacityUsage(partitionName, resourceConfig);
Review comment:
Even the constructor of Assignable Replica is not public. There is no need
we make the construction methods public. For now, I think it is overdesigned to
use builder pattern here.
We can revisit here if we are going to construct a replica object somewhere
else.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310244594
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
Review comment:
As comments above, I don't want to make this list immutable.
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
With regards,
Apache Git Services
[GitHub] [helix] jiajunwang commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
jiajunwang commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310244369
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
Review comment:
This is a test class. The abstract class might be used by the child in any
way. So if they want to add more resources to the list, it is fine.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310237287
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
Review comment:
Partation -> Partition
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310238349
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
Review comment:
Nit: also could do Assert.assertEquals(condition, error msg). One liner. I'm
fine with the way it is.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239674
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310236475
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310237911
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
Review comment:
This 3 is a magic number, so let's make it `NUM_FAULT_ZONES` and make it a
constant.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239447
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
Review comment:
Magic number -> constant
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239990
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239874
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239254
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
Review comment:
Magic number 9! Let's make this a constant: `NUM_INSTANCES`.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310239855
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310236475
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
+ public void testAlgorithmConstructor() {
+CardDealingAdjustmentAlgorithmV2 algorithm =
+new CardDealingAdjustmentAlgorithmV2(_topology, 3,
CardDealingAdjustmentAlgorithmV2.Mode.EVENNESS);
+Map instanceWeights = algorithm.getInstanceWeight();
+// verify weight is set correctly
+for (Map.Entry entry : instanceWeights.entrySet()) {
+ if (entry.getKey().getId() != entry.getValue()) {
+Assert.fail(String.format("%s %s should have weight of %s",
entry.getKey().getName(), entry.getKey().getId(),
+entry.getValue()));
+ }
+}
+Map faultZoneWeights = algorithm.getFaultZoneWeight();
+Map> faultZonePartationMap =
algorithm.getFaultZonePartitionMap();
+Set faultZones = faultZoneWeights.keySet();
+
+Assert.assertEquals(faultZoneWeights.size(), 3);
+Assert.assertEquals(faultZonePartationMap.keySet(), faultZones);
+
+long sum = 0;
+for (long weight : faultZoneWeights.values()) {
+ sum += weight;
+}
+// verify total weight is computed correct
+if (sum != algorithm.getTotalWeight()) {
+ Assert.fail(String.format("total weight %s != total weight of zones %s",
algorithm.getTotalWeight(), sum));
+}
+Map instanceFaultZone = algorithm.getInstanceFaultZone();
+Assert.assertEquals(instanceFaultZone.size(), 9);
+
+// verify zone mapping is correct
+for (Node zone : faultZones) {
+ long zoneId = zone.getId();
+ Set instanceIds = ImmutableSet.of(3 * zoneId + 1L, 3 * zoneId +
2L, 3 * zoneId + 3L);
+ List instanceNodes = zone.getChildren();
+ Assert.assertEquals(instanceNodes.size(), 3);
+ Set actualInstanceIds = new HashSet<>();
+ for (Node node : instanceNodes) {
+actualInstanceIds.add(node.getId());
+ }
+ Assert.assertEquals(actualInstanceIds, instanceIds);
+}
+ }
+
+ @DataProvider
+ public static Object[][] stableComputingVerification() {
+return new Object[][]{
+// replica, repeatTimes, seed, true: evenness false: less movement
+{1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100, true}, {1, 10, 0,
true}, {2, 10, 10, true},
+{3, 10, 100, true}, {1, 10, 0, true}, {2, 10, 10, true}, {3, 10, 100,
true}, {1, 10, 0, false},
+{2, 10, 10, false}, {3, 10, 100, false}, {1, 10, 0, false}, {2, 10,
10, false}, {3, 10, 100, false},
+{1, 10, 0, false}, {2, 10, 10, false}, {3, 10, 100, false}};
+ }
+
+ @Test(description = "Compute mapping multiple times, the mapping of
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310234688
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Node.java
##
@@ -36,6 +36,11 @@
public Node() { }
+ public Node(String name, long id) {
Review comment:
I think this is a minor point. Alternatively, you could just use the empty
constructor and call set methods but either way I think would work?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310234688
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Node.java
##
@@ -36,6 +36,11 @@
public Node() { }
+ public Node(String name, long id) {
Review comment:
I think this is a minor point. Alternatively, you could just use the empty
constructor and call set methods but either way I think would work?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310233448
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
Review comment:
I like how you consistently add the description. Here, though, could you add
a few lines on what properties are tested?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for
assigning partitions when instance weight …
URL: https://github.com/apache/helix/pull/369#discussion_r310233448
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/TestCardDealingAdjustmentAlgorithmV2.java
##
@@ -0,0 +1,328 @@
+package org.apache.helix.controller.rebalancer.strategy.crushMapping;
+
+/*
+ * 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.
+ */
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.controller.rebalancer.topology.InstanceNode;
+import org.apache.helix.controller.rebalancer.topology.Node;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.Mockito.*;
+
+
+public class TestCardDealingAdjustmentAlgorithmV2 {
+ private Topology _topology;
+ private int[][] _defaultFaultZones = new int[][]{{1, 2, 3}, // zone0: i1,
i2, i3
+ {4, 5, 6}, // zone1: i4, i5, i6
+ {7, 8, 9} // zone0: i7, i8, i9
+ };
+
+ @BeforeClass
+ public void setUpTopology() {
+_topology = mock(Topology.class);
+
when(_topology.getFaultZones()).thenReturn(createFaultZones(_defaultFaultZones));
+ }
+
+ @Test(description = "Verify a few properties after algorithm instance is
created")
Review comment:
I like how you consistently add the description. Here, though, could you add
a few lines on what properties are tested?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310232277 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: Yes, you would need to make the relevant fields `protected` (that was implied). `protected` should be good enough as long as we don't make it `public`. The point here is to avoid adding methods that aren't strictly part of the class logic :) 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310232277 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: Yes, you would need to make the relevant fields `protected`. That was implied. `protected` should be good enough as long as we don't make it `public`. The point here is to avoid adding methods that aren't strictly part of the class logic :) 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310230832
##
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java
##
@@ -208,13 +210,20 @@ protected void
chargeResource(StateTransitionThrottleConfig.RebalanceType rebala
*/
protected void chargeInstance(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
String instance) {
-if (_pendingTransitionAllowedPerInstance.containsKey(instance)
-&&
_pendingTransitionAllowedPerInstance.get(instance).containsKey(rebalanceType)) {
- chargeANYType(_pendingTransitionAllowedPerInstance.get(instance));
- Long instanceThrottle =
_pendingTransitionAllowedPerInstance.get(instance).get(rebalanceType);
- if (instanceThrottle > 0) {
-_pendingTransitionAllowedPerInstance.get(instance).put(rebalanceType,
instanceThrottle - 1);
- }
+charge(rebalanceType,
_pendingTransitionAllowedPerInstance.getOrDefault(instance, new HashMap<>()));
+ }
+
+ private void charge(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
+ Map quota) {
+if
(StateTransitionThrottleConfig.RebalanceType.NONE.equals(rebalanceType)) {
+ logger.error("Wrong rebalance type NONE as parameter");
+ return;
+}
+// if ANY type is present, decrement one else do nothing
+quota.computeIfPresent(StateTransitionThrottleConfig.RebalanceType.ANY,
(key, val) -> Math.max(0, val - 1));
Review comment:
I see what you were saying earlier. You could do `(type, quotaCount)` or
something similar. Point is to make the code a little more readable :)
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
With regards,
Apache Git Services
[GitHub] [helix] i3wangyi commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
i3wangyi commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310226396
##
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java
##
@@ -208,13 +210,20 @@ protected void
chargeResource(StateTransitionThrottleConfig.RebalanceType rebala
*/
protected void chargeInstance(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
String instance) {
-if (_pendingTransitionAllowedPerInstance.containsKey(instance)
-&&
_pendingTransitionAllowedPerInstance.get(instance).containsKey(rebalanceType)) {
- chargeANYType(_pendingTransitionAllowedPerInstance.get(instance));
- Long instanceThrottle =
_pendingTransitionAllowedPerInstance.get(instance).get(rebalanceType);
- if (instanceThrottle > 0) {
-_pendingTransitionAllowedPerInstance.get(instance).put(rebalanceType,
instanceThrottle - 1);
- }
+charge(rebalanceType,
_pendingTransitionAllowedPerInstance.getOrDefault(instance, new HashMap<>()));
+ }
+
+ private void charge(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
+ Map quota) {
+if
(StateTransitionThrottleConfig.RebalanceType.NONE.equals(rebalanceType)) {
+ logger.error("Wrong rebalance type NONE as parameter");
+ return;
+}
+// if ANY type is present, decrement one else do nothing
+quota.computeIfPresent(StateTransitionThrottleConfig.RebalanceType.ANY,
(key, val) -> Math.max(0, val - 1));
Review comment:
Look at the method parameter, these names are already taken
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939372
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
Review comment:
deduped
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213495
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
Review comment:
Explain please? What does it mean to reduce assignment?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310218704
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
+.remove(_partitionNames.get(0)));
+context.removePartitionFromFaultZone(_testFaultZoneId,
_resourceNames.get(0),
+_partitionNames.get(0));
+
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+ }
+
+ @Test(expectedExceptions = HelixException.class,
expectedExceptionsMessageRegExp = "Resource Resource1 already has a replica
from partition Partition1 in fault zone testZone")
+ public void testAssignAlreadyExist() throws IOException {
Review comment:
AssignExistingReplica?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310212807
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
+ResourceControllerDataProvider testCache =
Mockito.mock(ResourceControllerDataProvider.class);
+
+InstanceConfig testInstanceConfig = new InstanceConfig("testInstanceId");
+testInstanceConfig.setInstanceCapacityMap(_capacityDataMap);
+testInstanceConfig.addTag(_testInstanceTags.get(0));
+testInstanceConfig.setInstanceEnabledForPartition("TestResource",
"TestPartition", false);
+testInstanceConfig.setInstanceEnabled(true);
+testInstanceConfig.setZoneId(_testFaultZoneId);
+Map instanceConfigMap = new HashMap<>();
+instanceConfigMap.put(_testInstanceId, testInstanceConfig);
+when(testCache.getInstanceConfigMap()).thenReturn(instanceConfigMap);
+
+ClusterConfig testClusterConfig = new ClusterConfig("testClusterConfigId");
+testClusterConfig.setMaxPartitionsPerInstance(5);
+testClusterConfig.setDisabledInstances(Collections.emptyMap());
+testClusterConfig.setTopologyAwareEnabled(false);
+when(testCache.getClusterConfig()).thenReturn(testClusterConfig);
+
+LiveInstance testLiveInstance = new LiveInstance(_testInstanceId);
+testLiveInstance.setSessionId("testSessionId");
+Map liveInstanceMap = new HashMap<>();
+liveInstanceMap.put(_testInstanceId, testLiveInstance);
+when(testCache.getLiveInstances()).thenReturn(liveInstanceMap);
+
+CurrentState testCurrentStateResource1 = Mockito.mock(CurrentState.class);
+Map partitionStateMap1 = new HashMap<>();
+partitionStateMap1.put(_partitionNames.get(0), "MASTER");
+partitionStateMap1.put(_partitionNames.get(1), "SLAVE");
+
when(testCurrentStateResource1.getResourceName()).thenReturn(_resourceNames.get(0));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310170701
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
+.collect(Collectors.groupingBy(AssignableReplica::getResourceName,
Collectors.toSet()));
+
+// Index all the replicas to be assigned. Dedup the replica if two
instances have the same resource/partition/state
+_assignableReplicaIndex = assignableReplicas.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName, Collectors
+.toMap(AssignableReplica::toString, replica -> replica,
+(oldValue, newValue) -> oldValue)));
+
+_assignableNodeMap = assignableNodes.stream()
+.collect(Collectors.toMap(AssignableNode::getInstanceName, node ->
node));
+
+_baselineAssignment = baselineAssignment;
+_bestPossibleAssignment = bestPossibleAssignment;
+ }
+
+ public ClusterContext getContext() {
+return _clusterContext;
+ }
+
+ public Map getAssignableNodes() {
+return _assignableNodeMap;
+ }
+
+ public Map> getAssignableReplicas() {
+return _assignableReplicas;
+ }
+
+ public Map getBaseline() {
+return _baselineAssignment;
+ }
+
+ public Map getBestPossibleAssignment() {
+return _bestPossibleAssignment;
+ }
+
+ /**
+ * Propose the assignment to the cluster model.
Review comment:
Rephrase? This description doesn't make sense. Say "Try to make an
assignment for a given replica on the given instance. Note that this may not
show up in the final assignment - this assignment may be released and
re-assigned elsewhere."
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310223885
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterModel.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+public class TestClusterModel extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ Set generateNodes(ResourceControllerDataProvider testCache) {
+Set nodeSet = new HashSet<>();
+testCache.getInstanceConfigMap().values().stream().forEach(config ->
nodeSet
+.add(new AssignableNode(testCache, config.getInstanceName(),
Collections.emptyList(;
+return nodeSet;
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignableReplicas = generateReplicas(testCache);
+Set assignableNodes = generateNodes(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignableReplicas, 2);
+ClusterModel clusterModel =
+new ClusterModel(context, assignableReplicas, assignableNodes,
Collections.emptyMap(),
+Collections.emptyMap());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().isEmpty()));
+Assert.assertFalse(clusterModel.getAssignableNodes().values().stream()
+.anyMatch(n -> n.getCurrentAssignmentCount() != 0));
+
+// The initialization of the context, node and replication has been tested
separately. So for
+// cluster model, focus on testing the assignment and release.
+
+// Assign
+AssignableReplica replica = assignableReplicas.iterator().next();
+AssignableNode node = assignableNodes.iterator().next();
+clusterModel
+.assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+Assert.assertTrue(
+
clusterModel.getContext().getAssignmentForFaultZoneMap().get(node.getFaultZone())
+
.get(replica.getResourceName()).contains(replica.getPartitionName()));
+
Assert.assertTrue(node.getCurrentAssignmentsMap().get(replica.getResourceName())
+.contains(replica.getPartitionName()));
+
+// Assign a non-exist replication
+try {
+ clusterModel.assign("NOT-EXIST", replica.getPartitionName(),
replica.getReplicaState(),
+ node.getInstanceName());
+ Assert.fail("Assigning a non existing resource partition shall fail.");
+} catch (HelixException ex) {
+ // expected
+}
+
+// Assign a non-exist replication
+try {
+ clusterModel
+ .assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+ "NON-EXIST");
+ Assert.fail("Assigning a resource partition to a non existing instance
shall fail.");
+} catch (HelixException ex) {
+ // expected
+}
+
+// Release
+clusterModel
+.release(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().stream().allMatch(s -> s.isEmpty(;
Review comment:
Spell out one letter variables. They don't have to be long, but this is now
affecting readability. Especially it becomes confusing when nested maps are
involved.
Lambda expressions exist partly to reduce verbosity of Java but we shouldn't
sacrifice readability.
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309921368
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterContext.java
##
@@ -19,9 +19,100 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
- * This class tracks the global rebalance-related status of a Helix managed
cluster.
+ * This class tracks the rebalance-related global cluster status.
*/
-public class ClusterContext { }
+public class ClusterContext {
+ private final static float ERROR_MARGIN_FOR_ESTIMATED_MAX_COUNT = 1.1f;
+
+ // This estimation helps to ensure global partition count evenness
+ private final int _estimatedMaxPartitionCount;
+ // This estimation helps to ensure global top state replica count evenness
+ private final int _estimatedMaxTopStateCount;
+ // This estimation helps to ensure per-resource partition count evenness
+ private final Map _estimatedMaxPartitionByResource = new
HashMap<>();
+
+ // map{zoneName : map{resourceName : set(partitionNames)}}
+ private Map>> _assignmentForFaultZoneMap =
new HashMap<>();
+
+ /**
+ * Construct the cluster context based on the current instance status.
+ *
+ * @param replicaSetAll the partition replicas that are managed by the
rebalancer
+ * @param instanceCount The count of all the active instances that can be
used to host partitions.
+ */
+ ClusterContext(Set replicaSet, int instanceCount) {
+int totalReplicas = 0;
+int totalTopStateReplicas = 0;
+
+for (Map.Entry> entry : replicaSet.stream()
+
.collect(Collectors.groupingBy(AssignableReplica::getResourceName)).entrySet())
{
+ int replicas = entry.getValue().size();
+ totalReplicas += replicas;
+
+ int replicaCnt = Math.max(1, estimateAvgReplicaCount(replicas,
instanceCount));
+ _estimatedMaxPartitionByResource.put(entry.getKey(), replicaCnt);
+
+ totalTopStateReplicas +=
+
entry.getValue().stream().filter(AssignableReplica::isReplicaTopState).count();
+}
+
+_estimatedMaxPartitionCount = estimateAvgReplicaCount(totalReplicas,
instanceCount);
+_estimatedMaxTopStateCount =
estimateAvgReplicaCount(totalTopStateReplicas, instanceCount);
+ }
+
+ public Map>> getAssignmentForFaultZoneMap() {
+return _assignmentForFaultZoneMap;
+ }
+
+ public int getEstimatedMaxPartitionCount() {
+return _estimatedMaxPartitionCount;
+ }
+
+ public int getEstimatedMaxPartitionByResource(String resourceName) {
+return _estimatedMaxPartitionByResource.get(resourceName);
+ }
+
+ public int getEstimatedMaxTopStateCount() {
+return _estimatedMaxTopStateCount;
+ }
+
+ public Set getPartitionsForResourceAndFaultZone(String faultZoneId,
String resourceName) {
Review comment:
Minor: Keep the order consistent in both the name and arguments - resource
and fault zone
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213728
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310214870
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939522
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
Review comment:
Revise this replica key to resource_partition_state? Or we could link the
method?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#discussion_r310217077 ## File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java ## @@ -0,0 +1,93 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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. + */ + +import org.apache.helix.HelixException; +import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider; +import org.testng.Assert; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.stream.Collectors.toSet; Review comment: Nit: use Collectors.toSet inline? I don't really care either way but we usually don't import static methods? 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310216570
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
Review comment:
leaved -> left
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310216339
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
+Map capacityDataMapResource1 = new HashMap<>();
+capacityDataMapResource1.put("item1", 3);
+capacityDataMapResource1.put("item2", 6);
+ResourceConfig testResourceConfigResource = new
ResourceConfig(resourceName);
+testResourceConfigResource.setPartitionCapacityMap(
+Collections.singletonMap(ResourceConfig.DEFAULT_PARTITION_KEY,
capacityDataMapResource1));
+
+String partitionName = partitionNamePrefix + 1;
+AssignableReplica replica =
+new AssignableReplica(testResourceConfigResource, partitionName,
masterState,
+masterPriority);
+Assert.assertEquals(replica.getResourceName(), resourceName);
Review comment:
Can this series of asserts could be put into a private helper method called
verifyReplica or something? Too much repeated code?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310214018
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310162365
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
+ * Note that the replicas in this list shall
not be included while initializing the context and assignable nodes.
+ * @param assignableNodesThe active instances.
+ * @param baselineAssignment The recorded baseline assignment.
+ * @param bestPossibleAssignment The current best possible assignment.
+ */
+ ClusterModel(ClusterContext clusterContext, Set
assignableReplicas,
+ Set assignableNodes, Map
baselineAssignment,
+ Map bestPossibleAssignment) {
+_clusterContext = clusterContext;
+
+// Save all the to be assigned replication
+_assignableReplicas = assignableReplicas.stream()
Review comment:
Nit: just to avoid confusion, what do you think about renaming
assignableReplicas to assignableReplicaMap? That's what you seem to be doing
assignableNodeMap/assignableNodes.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310215326
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
Review comment:
replication -> replica?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r309939616
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
Review comment:
What does the string represent?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on issue #362: The WAGED rebalancer cluster model implementation
narendly commented on issue #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#issuecomment-517781121 This is a big PR! I think it would be a good idea to keep the size of PRs smaller if possible. 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310221038
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterModel.java
##
@@ -0,0 +1,108 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+public class TestClusterModel extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ Set generateNodes(ResourceControllerDataProvider testCache) {
+Set nodeSet = new HashSet<>();
+testCache.getInstanceConfigMap().values().stream().forEach(config ->
nodeSet
+.add(new AssignableNode(testCache, config.getInstanceName(),
Collections.emptyList(;
+return nodeSet;
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignableReplicas = generateReplicas(testCache);
+Set assignableNodes = generateNodes(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignableReplicas, 2);
+ClusterModel clusterModel =
+new ClusterModel(context, assignableReplicas, assignableNodes,
Collections.emptyMap(),
+Collections.emptyMap());
+
+
Assert.assertTrue(clusterModel.getContext().getAssignmentForFaultZoneMap().values().stream()
+.allMatch(v -> v.values().isEmpty()));
+Assert.assertFalse(clusterModel.getAssignableNodes().values().stream()
+.anyMatch(n -> n.getCurrentAssignmentCount() != 0));
+
+// The initialization of the context, node and replication has been tested
separately. So for
+// cluster model, focus on testing the assignment and release.
+
+// Assign
+AssignableReplica replica = assignableReplicas.iterator().next();
+AssignableNode node = assignableNodes.iterator().next();
+clusterModel
+.assign(replica.getResourceName(), replica.getPartitionName(),
replica.getReplicaState(),
+node.getInstanceName());
+
+Assert.assertTrue(
+
clusterModel.getContext().getAssignmentForFaultZoneMap().get(node.getFaultZone())
+
.get(replica.getResourceName()).contains(replica.getPartitionName()));
+
Assert.assertTrue(node.getCurrentAssignmentsMap().get(replica.getResourceName())
+.contains(replica.getPartitionName()));
+
+// Assign a non-exist replication
Review comment:
nonexistent
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213569
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+
+Set expectedAssignmentSet1 = new
HashSet<>(_partitionNames.subList(0, 2));
+Set expectedAssignmentSet2 = new
HashSet<>(_partitionNames.subList(2, 4));
+Map> expectedAssignment = new HashMap<>();
+expectedAssignment.put("Resource1", expectedAssignmentSet1);
+expectedAssignment.put("Resource2", expectedAssignmentSet2);
+Map expectedCapacityMap = new HashMap<>();
+expectedCapacityMap.put("item1", 4);
+expectedCapacityMap.put("item2", 8);
+expectedCapacityMap.put("item3", 30);
+
+
+AssignableNode assignableNode = new AssignableNode(testCache,
_testInstanceId, assignmentSet);
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 4);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 16.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
Assert.assertTrue(assignableNode.getCurrentCapacity().equals(expectedCapacityMap));
+
+// Test 2 - reduce assignment
+AssignableReplica removingReplica =
+new
AssignableReplica(testCache.getResourceConfig(_resourceNames.get(1)),
+_partitionNames.get(2), "MASTER", 1);
+
expectedAssignment.get(_resourceNames.get(1)).remove(_partitionNames.get(2));
+expectedCapacityMap.put("item1", 9);
+expectedCapacityMap.put("item2", 18);
+
+assignableNode.release(removingReplica);
+
+
Assert.assertTrue(assignableNode.getCurrentAssignmentsMap().equals(expectedAssignment));
+Assert.assertEquals(assignableNode.getCurrentAssignmentCount(), 3);
+Assert.assertEquals(assignableNode.getHighestCapacityUtilization(), 11.0 /
20.0, 0.005);
+
Assert.assertTrue(assignableNode.getMaxCapacity().equals(_capacityDataMap));
+Assert.assertEquals(assignableNode.getMaxPartition(), 5);
+Assert.assertEquals(assignableNode.getInstanceTags(), _testInstanceTags);
+Assert.assertEquals(assignableNode.getFaultZone(), _testFaultZoneId);
+
Assert.assertTrue(assignableNode.getDisabledPartitionsMap().equals(_disabledPartitionsMap));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310218571
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
+.addPartitionToFaultZone(_testFaultZoneId, r.getResourceName(),
r.getPartitionName()));
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
expectedFaultZoneMap);
+
+// release
+
Assert.assertTrue(expectedFaultZoneMap.get(_testFaultZoneId).get(_resourceNames.get(0))
Review comment:
Any reason why you're not using `assign` and `release` functions? instead of
manually removing them from maps?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310216686
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
Review comment:
Add some description about your tests!
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation URL: https://github.com/apache/helix/pull/362#discussion_r310215210 ## File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java ## @@ -0,0 +1,100 @@ +package org.apache.helix.controller.rebalancer.waged.model; + +/* + * 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. + */ + +import org.apache.helix.model.ResourceConfig; +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY; Review comment: Nit: instead of importing this, can't we just do StateModelDefinition.TOP_STATE_PRIORITY? 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213021
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/AbstractTestClusterModel.java
##
@@ -0,0 +1,161 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.annotations.BeforeClass;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public abstract class AbstractTestClusterModel {
+ protected String _testInstanceId;
+ protected List _resourceNames;
+ protected List _partitionNames;
+ protected Map _capacityDataMap;
+ protected Map> _disabledPartitionsMap;
+ protected List _testInstanceTags;
+ protected String _testFaultZoneId;
+
+ @BeforeClass
+ public void initialize() {
+_testInstanceId = "testInstanceId";
+_resourceNames = new ArrayList<>();
+_resourceNames.add("Resource1");
+_resourceNames.add("Resource2");
+_partitionNames = new ArrayList<>();
+_partitionNames.add("Partition1");
+_partitionNames.add("Partition2");
+_partitionNames.add("Partition3");
+_partitionNames.add("Partition4");
+_capacityDataMap = new HashMap<>();
+_capacityDataMap.put("item1", 20);
+_capacityDataMap.put("item2", 40);
+_capacityDataMap.put("item3", 30);
+List disabledPartitions = new ArrayList<>();
+disabledPartitions.add("TestPartition");
+_disabledPartitionsMap = new HashMap<>();
+_disabledPartitionsMap.put("TestResource", disabledPartitions);
+_testInstanceTags = new ArrayList<>();
+_testInstanceTags.add("TestTag");
+_testFaultZoneId = "testZone";
+ }
+
+ protected ResourceControllerDataProvider setupClusterDataCache() throws
IOException {
+ResourceControllerDataProvider testCache =
Mockito.mock(ResourceControllerDataProvider.class);
+
+InstanceConfig testInstanceConfig = new InstanceConfig("testInstanceId");
+testInstanceConfig.setInstanceCapacityMap(_capacityDataMap);
+testInstanceConfig.addTag(_testInstanceTags.get(0));
+testInstanceConfig.setInstanceEnabledForPartition("TestResource",
"TestPartition", false);
+testInstanceConfig.setInstanceEnabled(true);
+testInstanceConfig.setZoneId(_testFaultZoneId);
+Map instanceConfigMap = new HashMap<>();
+instanceConfigMap.put(_testInstanceId, testInstanceConfig);
+when(testCache.getInstanceConfigMap()).thenReturn(instanceConfigMap);
+
+ClusterConfig testClusterConfig = new ClusterConfig("testClusterConfigId");
+testClusterConfig.setMaxPartitionsPerInstance(5);
+testClusterConfig.setDisabledInstances(Collections.emptyMap());
+testClusterConfig.setTopologyAwareEnabled(false);
+when(testCache.getClusterConfig()).thenReturn(testClusterConfig);
+
+LiveInstance testLiveInstance = new LiveInstance(_testInstanceId);
+testLiveInstance.setSessionId("testSessionId");
+Map liveInstanceMap = new HashMap<>();
+liveInstanceMap.put(_testInstanceId, testLiveInstance);
+when(testCache.getLiveInstances()).thenReturn(liveInstanceMap);
+
+CurrentState testCurrentStateResource1 = Mockito.mock(CurrentState.class);
+Map partitionStateMap1 = new HashMap<>();
+partitionStateMap1.put(_partitionNames.get(0), "MASTER");
+partitionStateMap1.put(_partitionNames.get(1), "SLAVE");
+
when(testCurrentStateResource1.getResourceName()).thenReturn(_resourceNames.get(0));
+
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310215940
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableReplica.java
##
@@ -0,0 +1,100 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.model.ResourceConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.helix.model.StateModelDefinition.TOP_STATE_PRIORITY;
+
+public class TestAssignableReplica {
+ String resourceName = "Resource";
+ String partitionNamePrefix = "partition";
+ String masterState = "Master";
+ int masterPriority = TOP_STATE_PRIORITY;
+ String slaveState = "Slave";
+ int slavePriority = 2;
+
+ @Test
+ public void testConstructRepliaWithResourceConfig() throws IOException {
+// Init assignable replication with a basic config object
+Map capacityDataMapResource1 = new HashMap<>();
+capacityDataMapResource1.put("item1", 3);
+capacityDataMapResource1.put("item2", 6);
+ResourceConfig testResourceConfigResource = new
ResourceConfig(resourceName);
+testResourceConfigResource.setPartitionCapacityMap(
+Collections.singletonMap(ResourceConfig.DEFAULT_PARTITION_KEY,
capacityDataMapResource1));
+
+String partitionName = partitionNamePrefix + 1;
+AssignableReplica replica =
+new AssignableReplica(testResourceConfigResource, partitionName,
masterState,
+masterPriority);
+Assert.assertEquals(replica.getResourceName(), resourceName);
+Assert.assertEquals(replica.getPartitionName(), partitionName);
+Assert.assertEquals(replica.getReplicaState(), masterState);
+Assert.assertEquals(replica.getStatePriority(), masterPriority);
+Assert.assertTrue(replica.isReplicaTopState());
+Assert.assertEquals(replica.getCapacity(), capacityDataMapResource1);
+Assert.assertEquals(replica.getResourceInstanceGroupTag(), null);
+Assert.assertEquals(replica.getResourceMaxPartitionsPerInstance(),
Integer.MAX_VALUE);
+
+// Modify the config and initial more replications.
Review comment:
initial -> initialize
replications -> replicas
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310161710
##
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/model/ClusterModel.java
##
@@ -19,9 +19,131 @@
* under the License.
*/
+import org.apache.helix.HelixException;
+import org.apache.helix.model.IdealState;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
/**
- * A placeholder before we have the implementation.
- *
* This class wraps the required input for the rebalance algorithm.
*/
-public class ClusterModel { }
+public class ClusterModel {
+ private final ClusterContext _clusterContext;
+ // Map to track all the assignable replications. >
+ private final Map> _assignableReplicas;
+ // The index to find the replication information with a certain state.
>
+ // Note that the identical replicas are dedupe in the index.
+ private final Map>
_assignableReplicaIndex;
+ private final Map _assignableNodeMap;
+
+ // Records about the previous assignment
+ private final Map _baselineAssignment;
+ private final Map _bestPossibleAssignment;
+
+ /**
+ * @param clusterContext The initialized cluster context.
+ * @param assignableReplicas The replications to be assigned.
Review comment:
replications -> replicas
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310218199
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestClusterContext.java
##
@@ -0,0 +1,93 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static java.util.stream.Collectors.toSet;
+
+public class TestClusterContext extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
+ClusterContext context = new ClusterContext(assignmentSet, 2);
+
+// Note that we leaved some margin for the max estimation.
+Assert.assertEquals(context.getEstimatedMaxPartitionCount(), 3);
+Assert.assertEquals(context.getEstimatedMaxTopStateCount(), 2);
+Assert.assertEquals(context.getAssignmentForFaultZoneMap(),
Collections.emptyMap());
+for (String resourceName : _resourceNames) {
+
Assert.assertEquals(context.getEstimatedMaxPartitionByResource(resourceName),
2);
+ Assert.assertEquals(
+ context.getPartitionsForResourceAndFaultZone(_testFaultZoneId,
resourceName),
+ Collections.emptySet());
+}
+
+// Assign
+Map>> expectedFaultZoneMap = Collections
+.singletonMap(_testFaultZoneId,
assignmentSet.stream().collect(Collectors
+.groupingBy(AssignableReplica::getResourceName,
+Collectors.mapping(AssignableReplica::getPartitionName,
toSet();
+
+assignmentSet.stream().forEach(r -> context
Review comment:
Nit: spell out r -> assignableReplica for readability
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #362: The WAGED rebalancer cluster model implementation
narendly commented on a change in pull request #362: The WAGED rebalancer
cluster model implementation
URL: https://github.com/apache/helix/pull/362#discussion_r310213353
##
File path:
helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/model/TestAssignableNode.java
##
@@ -0,0 +1,203 @@
+package org.apache.helix.controller.rebalancer.waged.model;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.integration.task.TaskTestBase;
+import org.apache.helix.model.BuiltInStateModelDefinitions;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.ResourceConfig;
+import org.mockito.Mockito;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.mockito.Mockito.when;
+
+public class TestAssignableNode extends AbstractTestClusterModel {
+ @BeforeClass
+ public void initialize() {
+super.initialize();
+ }
+
+ @Test
+ public void testNormalUsage() throws IOException {
+ResourceControllerDataProvider testCache = setupClusterDataCache();
+Set assignmentSet = generateReplicas(testCache);
+
+// Test 1 - initialization
Review comment:
Explain?
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
With regards,
Apache Git Services
[GitHub] [helix] alirezazamani closed issue #370: Remove unnecessary touch logic when updating ResourceConfig
alirezazamani closed issue #370: Remove unnecessary touch logic when updating ResourceConfig URL: https://github.com/apache/helix/issues/370 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 With regards, Apache Git Services
helix - Build # 1638 - Still Failing
The Apache Jenkins build system has built helix (build #1638) Status: Still Failing Check console output at https://builds.apache.org/job/helix/1638/ to view the results.
[GitHub] [helix] dasahcc merged pull request #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines
dasahcc merged pull request #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines URL: https://github.com/apache/helix/pull/371 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 With regards, Apache Git Services
[GitHub] [helix] alirezazamani commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines
alirezazamani commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines URL: https://github.com/apache/helix/pull/371#issuecomment-517754322 This PR is ready to be merged, approved by @jiajunwang and @narendly. 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 With regards, Apache Git Services
[GitHub] [helix] alirezazamani commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines
alirezazamani commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines URL: https://github.com/apache/helix/pull/371#issuecomment-517754114 These changes have been covered by tests. I also check it manually and the pipeline is triggered. 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight …
narendly commented on a change in pull request #369: Add the workaround fix for assigning partitions when instance weight … URL: https://github.com/apache/helix/pull/369#discussion_r310154419 ## File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/strategy/crushMapping/CardDealingAdjustmentAlgorithmV2.java ## @@ -55,6 +74,27 @@ public CardDealingAdjustmentAlgorithmV2(Topology topology, int replica, Mode mod } } + // expose internal fields only for testing purpose Review comment: As I mentioned in another PR, one trick you can use is to create a MockCardDealingAdjustmentAlgorithmV2 that extends this class and that contains these methods. That way you should be able to access all protected fields, and use the mock class for unit testing. 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310153372 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: I am not 100% comfortable with adding methods just for testing/debugging. This is a potential vulnerability in scoping. Here's a trick you can do (I have done this in some tests): Instead of adding methods (that shouldn't otherwise be used other than testing purposes), create a child class (for example, `MockStateTranisitonThrottleController`) that extends this class and just add these test/debug methods. That way, in your unit tests, you could use the mock class and still utilize these methods that expose internal variables, rather than adding methods directly to the main class. Do you think that would work? 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config URL: https://github.com/apache/helix/pull/333#discussion_r310153372 ## File path: helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java ## @@ -99,6 +99,21 @@ public StateTransitionThrottleController(Set resources, ClusterConfig cl } } + // The getter methods are used for debugging and testing purpose Review comment: I am not 100% comfortable with adding methods just for this purpose. Here's a trick you can do (I have done this in some tests): Instead of adding methods (that shouldn't otherwise be used other than testing purposes), create a child class (for example, `MockStateTranisitonThrottleController`) that extends this class and just add these test/debug methods. That way, in your unit tests, you could use the mock class and still utilize these methods that expose internal variables, rather than adding methods directly to the main class. Do you think that would work? 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 With regards, Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310150104
##
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/StateTransitionThrottleController.java
##
@@ -208,13 +210,20 @@ protected void
chargeResource(StateTransitionThrottleConfig.RebalanceType rebala
*/
protected void chargeInstance(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
String instance) {
-if (_pendingTransitionAllowedPerInstance.containsKey(instance)
-&&
_pendingTransitionAllowedPerInstance.get(instance).containsKey(rebalanceType)) {
- chargeANYType(_pendingTransitionAllowedPerInstance.get(instance));
- Long instanceThrottle =
_pendingTransitionAllowedPerInstance.get(instance).get(rebalanceType);
- if (instanceThrottle > 0) {
-_pendingTransitionAllowedPerInstance.get(instance).put(rebalanceType,
instanceThrottle - 1);
- }
+charge(rebalanceType,
_pendingTransitionAllowedPerInstance.getOrDefault(instance, new HashMap<>()));
+ }
+
+ private void charge(StateTransitionThrottleConfig.RebalanceType
rebalanceType,
+ Map quota) {
+if
(StateTransitionThrottleConfig.RebalanceType.NONE.equals(rebalanceType)) {
+ logger.error("Wrong rebalance type NONE as parameter");
+ return;
+}
+// if ANY type is present, decrement one else do nothing
+quota.computeIfPresent(StateTransitionThrottleConfig.RebalanceType.ANY,
(key, val) -> Math.max(0, val - 1));
Review comment:
Sorry, I don't quite understand. Does `(rebalanceType, quota) -> Math.max(0,
quota - 1)` not work? Are you saying it's not important to make code
self-descriptive? The whole point is to make it so that it's easier to
understand for people who are not familiar with what we're doing here?
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #333: Fix issue when client only sets ANY at cluster level throttle config
narendly commented on a change in pull request #333: Fix issue when client only
sets ANY at cluster level throttle config
URL: https://github.com/apache/helix/pull/333#discussion_r310150588
##
File path:
helix-core/src/test/java/org/apache/helix/integration/TestPartitionMovementThrottle.java
##
@@ -279,13 +332,15 @@ public void testResourceThrottleWithDelayRebalancer() {
Assert.assertTrue(_clusterVerifier.verifyByPolling());
for (String db : _dbs) {
- validateThrottle(DelayedTransition.getResourcePatitionTransitionTimes(),
db, 2);
+ int maxInParallel =
getMaxParallelTransitionCount(DelayedTransition.getResourcePatitionTransitionTimes(),
db);
+ System.out.println(
+ "MaxInParallel: " + maxInParallel + " maxPendingTransition: " + 2);
+ Assert.assertTrue(maxInParallel <= 2, "Throttle condition does not meet
for " + db);
}
}
- private void validateThrottle(
Review comment:
@i3wangyi You make a great point. Thanks for clarifying and I believe this
is the correct thing to do.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on a change in pull request #359: Dynamically change the processor thread name when consuming event
narendly commented on a change in pull request #359: Dynamically change the
processor thread name when consuming event
URL: https://github.com/apache/helix/pull/359#discussion_r310147891
##
File path:
helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##
@@ -1165,7 +1163,11 @@ public void run() {
+ _processorName);
while (!isInterrupted()) {
try {
- handleEvent(_eventBlockingQueue.take(), _cache);
+ ClusterEvent newClusterEvent = _eventBlockingQueue.take();
Review comment:
I don't think it's too much for the simplicity of the code, but I do think
that it will be more work for you to make that happen.
I don't know if I follow "And logically, ClusterEvent shouldn't know the
existence of threadName even queue object".
At any rate, the point still stands - it is always important to minimize the
amount of objects being created since we need to be conscious of memory
pressure. If you have other thoughts, please post them here.
I am fine with leaving this item as a **TODO** in the codebase and creating
a corresponding issue.
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
With regards,
Apache Git Services
[GitHub] [helix] narendly commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines
narendly commented on issue #371: Remove unnecessary touch logic issues that forces triggering Helix pipelines URL: https://github.com/apache/helix/pull/371#issuecomment-517712533 You could do https://github.com/apache/helix/wiki/Pull-Request-Check-In-Steps to get this checked in. As discussed offline, take your time to test it out locally :) 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 With regards, Apache Git Services
