You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/01/14 22:57:14 UTC

[GitHub] [helix] qqu0127 opened a new pull request #1935: Implement java API and utils for virtual topology group

qqu0127 opened a new pull request #1935:
URL: https://github.com/apache/helix/pull/1935


   ### Description
   
   - [x] Here are some details about my PR, including screenshots of any UI changes:
   
   Implement the java endpoint for adding virtual topology group for a cluster. The entry point is `VirtualTopologyGroupService#addVirtualTopologyGroup`.
   This provides an extra layer in cluster topology named "virtual_zone" so that we have more control over it and can change the number and assignment on the fly. This also decouples the setup from physical fault domain so it's no longer a restriction.
   
   This API is only expected to be invoked in cloud environment where `cloudConfig.isCloudEnabled()` is true.
   Besides, a new cluster config is introduced: `VIRTUAL_GROUP_ENABLED` to enable this feature.
   
   An algorithm for assigning virtual topology group is introduced in `VirtualTopologyGroupStrategy`. It takes the cluster topology and assigns virtual zone for each instance under a cluster. Instance configs are then updated asynchronously. 
   The operation is NOT transactional, but there are internal retry logic, maintenance mode will also be enabled during the process.
   
   TODO in the following commits and PRs:
   1. Implement REST API
   2. Integration test
   3. Documentation and user manual
   
   ### Tests
   
   - [x] The following tests are written for this issue:
   `TestVirtualTopologyGroup`
   `TestVirtualTopologyGroupService`
   
   - The following is the result of the "mvn test" command on the appropriate module:
   [INFO] Tests run: 196, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 208.515 s - in TestSuite
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 196, Failures: 0, Errors: 0, Skipped: 0
   [INFO] 
   [INFO] 
   [INFO] --- jacoco-maven-plugin:0.8.6:report (generate-code-coverage-report) @ helix-rest ---
   [INFO] Loading execution data file /Users/qqu/workspace/qqu-helix/helix-rest/target/jacoco.exec
   [INFO] Analyzed bundle 'Apache Helix :: Restful Interface' with 87 classes
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD SUCCESS
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time:  03:34 min
   [INFO] Finished at: 2022-01-14T14:25:42-08:00
   [INFO] ------------------------------------------------------------------------
   
   ### Commits
   
   - My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Code Quality
   
   - My diff has been formatted using helix-style.xml 
   (helix-style-intellij.xml if IntelliJ IDE is used)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r798998573



##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -52,6 +52,7 @@
     FAULT_ZONE_TYPE, // the type in which isolation should be applied on when Helix places the
     // replicas from same partition.
     TOPOLOGY_AWARE_ENABLED, // whether topology aware rebalance is enabled.
+    VIRTUAL_GROUP_ENABLED, // whether virtual topology group is enabled

Review comment:
       As discussed offline, there is a design change and we are discarding `VIRTUAL_GROUP_ENABLED`. Rollback is not a concern because there is always a fixed topology config on cloud environment.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] xyuanlu commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r787026907



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.controller.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {

Review comment:
       Thanks for the info. Let's keep it this way then.  




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788106907



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;

Review comment:
       How would that work? If we have 24 instances over 5 HDF, to assign 7 virtual groups. Notice `ceil(24 / 7) == 4` and `24 / 4 == 6`, 24 instances are effectively only assigned to 6 groups.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] xyuanlu commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r785253766



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.controller.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {

Review comment:
       nit: how about 
   if ( results.stream().anyMatch(res-> !res) ) {
      throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));
   }




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r799971895



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#ENTER_MAINTENANCE_MODE} is optional.
+   *                     if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                     completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean enterMaintenanceMode = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.ENTER_MAINTENANCE_MODE, "true"));

Review comment:
       In this case, this config should be named as "AUTO_ENTER_MAINTENANCE_MODE". But it is still a bit weird that this config has a default value as "true". Normally if a boolean config is not set, the default value is false. You can think about whether you want to change it to "DISABLE_AUTO_ENTER_MAINTENANCE_MODE". Users need to manually set it to true if they would like to control it themselves.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788059969



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;
+
+      // instances.size = instancePerGroup * numGroups + residual
+      // step 1, continuously assign following groupInd order until entering residual section
+      for (int instanceInd = 0; instanceInd < instancesPerGroup * numGroups; instanceInd++) {
+        int groupIndex = instanceInd / instancesPerGroup;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      // step 2, assign the residuals, either stick to the last assigned group or round-robin to all groups.
+      for (int instanceInd = instancesPerGroup * numGroups; instanceInd < sortedInstances.size(); instanceInd++) {
+        int groupIndex = numGroups <= zoneMapping.keySet().size()
+            ? numGroups - 1
+            : instanceInd % numGroups;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      return ImmutableMap.copyOf(assignment);
+    }
+  };
+
+  /**
+   * Compute the assignment for each virtual topology group.
+   *
+   * @param numGroups number of the virtual groups
+   * @param virtualGroupName virtual group name
+   * @param zoneMapping current zone mapping from zoneId to instanceIds
+   * @return the assignment as mapping from virtual group ID to instanceIds
+   */
+  public abstract Map<String, Set<String>> computeAssignment(
+      int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping);
+
+  private static String computeVirtualGroupId(int groupIndex, String virtualGroupName) {
+    return virtualGroupName + "_" + groupIndex;

Review comment:
       What's your suggestion for constructing the virtual groupId? I can go with a string template for naming, but don't think that's much difference than this.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788111723



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;

Review comment:
       It's possible to have multiple strategies in the future and to be configurable, but right now there is only one. There is also discussion with Hunter on this, see https://github.com/apache/helix/pull/1935#discussion_r786372019




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788122216



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");

Review comment:
       In my understanding this feature is specifically designed for Azure, where hard fault zone is out of our control. For on-prem, we have a way to adjust FZ count. @junkaixue may have more information on why this is designed for only azure?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] jiajunwang commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788057091



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;
+
+      // instances.size = instancePerGroup * numGroups + residual
+      // step 1, continuously assign following groupInd order until entering residual section
+      for (int instanceInd = 0; instanceInd < instancesPerGroup * numGroups; instanceInd++) {
+        int groupIndex = instanceInd / instancesPerGroup;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      // step 2, assign the residuals, either stick to the last assigned group or round-robin to all groups.
+      for (int instanceInd = instancesPerGroup * numGroups; instanceInd < sortedInstances.size(); instanceInd++) {
+        int groupIndex = numGroups <= zoneMapping.keySet().size()
+            ? numGroups - 1
+            : instanceInd % numGroups;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      return ImmutableMap.copyOf(assignment);
+    }
+  };
+
+  /**
+   * Compute the assignment for each virtual topology group.
+   *
+   * @param numGroups number of the virtual groups
+   * @param virtualGroupName virtual group name
+   * @param zoneMapping current zone mapping from zoneId to instanceIds
+   * @return the assignment as mapping from virtual group ID to instanceIds
+   */
+  public abstract Map<String, Set<String>> computeAssignment(
+      int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping);
+
+  private static String computeVirtualGroupId(int groupIndex, String virtualGroupName) {
+    return virtualGroupName + "_" + groupIndex;

Review comment:
       nit, avoid hard 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] jiajunwang commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r789028024



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;
+
+      // instances.size = instancePerGroup * numGroups + residual
+      // step 1, continuously assign following groupInd order until entering residual section
+      for (int instanceInd = 0; instanceInd < instancesPerGroup * numGroups; instanceInd++) {
+        int groupIndex = instanceInd / instancesPerGroup;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      // step 2, assign the residuals, either stick to the last assigned group or round-robin to all groups.
+      for (int instanceInd = instancesPerGroup * numGroups; instanceInd < sortedInstances.size(); instanceInd++) {
+        int groupIndex = numGroups <= zoneMapping.keySet().size()
+            ? numGroups - 1
+            : instanceInd % numGroups;
+        String groupId = computeVirtualGroupId(groupIndex, virtualGroupName);
+        assignment.putIfAbsent(groupId, new HashSet<>());
+        assignment.get(groupId).add(sortedInstances.get(instanceInd));
+      }
+      return ImmutableMap.copyOf(assignment);
+    }
+  };
+
+  /**
+   * Compute the assignment for each virtual topology group.
+   *
+   * @param numGroups number of the virtual groups
+   * @param virtualGroupName virtual group name
+   * @param zoneMapping current zone mapping from zoneId to instanceIds
+   * @return the assignment as mapping from virtual group ID to instanceIds
+   */
+  public abstract Map<String, Set<String>> computeAssignment(
+      int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping);
+
+  private static String computeVirtualGroupId(int groupIndex, String virtualGroupName) {
+    return virtualGroupName + "_" + groupIndex;

Review comment:
       I can see you did 
   
   > private static final String DOMAIN_FIELD_SPLITTER = ",";
   > private static final String DOMAIN_VALUE_JOINER = "=";
   
   for the InstanceConfig. Then why hardcode here? It's better to keep the same style.

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       I agree strategy fits well. But due to the existing usage, I would say, "arrangement" or "scheme" for this class?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");

Review comment:
       As we discussed offline, the cloud here does not necessarily mean a real cloud platform. It solely means that Helix gets topology information from the participant under this scenario. Meaning the topology information is automatically provided instead of manually populated. So this restriction makes sense here.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);

Review comment:
       I was thinking about negative numbers or zero. But if the group number is larger than the node count, can we still assign it?
   It's also OK if you let the compute class throw exceptions. But please ensure the exception is understandable for our customers. IndexOutOfBoundsException is not acceptable. 

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;

Review comment:
       Not exactly, the reason you get the conclusion of 6 groups is that you sort the instance and compute groupIndex with "instanceInd / instancesPerGroup". My original idea is that we can directly use mod to get the index.
   However, according to what we have discussed yesterday, it might be problematic to assign instances of one physical zone to multiple virtual groups. It may effectively downgrade fault tolerance. So I assume you will have to change the design here. Let's discuss further when your new design is ready : )

##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -52,6 +52,7 @@
     FAULT_ZONE_TYPE, // the type in which isolation should be applied on when Helix places the
     // replicas from same partition.
     TOPOLOGY_AWARE_ENABLED, // whether topology aware rebalance is enabled.
+    VIRTUAL_GROUP_ENABLED, // whether virtual topology group is enabled

Review comment:
       As we discussed offline yesterday, it is a must as long as we still want to keep the physical topology somewhere and may want to provide a way to roll back.
   
   But logic-wise, it introduced more dependencies. For example, what if I enable virtual group without enabling topology awareness, does it even makes sense? What about when cloud features are not enabled? I can foresee many questions from our users.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {

Review comment:
       > we shouldn't proceed if maintenance mode is already on?
   
   Yes, or you don't exit maintenance mode at the end.

##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtualZone";
+  public static final String VIRTUAL_TOPOLOGY = new StringBuilder(AzureConstants.AZURE_TOPOLOGY)

Review comment:
       I don't believe the high-level design of VirtualTopologyGroup is for Azure only. It should be determined by the caller when they call the Rest API.
   As we discussed yesterday, whenever a cluster is "cloud-enabled", it is supporting virtual topology. And for Helix, "cloud-enabled" does not mean a specific cloud platform or even public cloud. As long as the cluster nodes can provide the necessary information, it is supporting necessary cloud feature from Helix perspective.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r798999850



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.

Review comment:
       Discussed offline, we implemented a different algorithm. Resolved this in separate PR -- https://github.com/apache/helix/pull/1948




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 merged pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 merged pull request #1935:
URL: https://github.com/apache/helix/pull/1935


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r802050133



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,194 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.

Review comment:
       This doc does not convey anything other than the class name. If you do not have anywhere else document what is virtual group, you can consider put it here, concisely. 

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,194 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. We override {@link ClusterConfig.ClusterConfigProperty#TOPOLOGY} and
+   * {@link ClusterConfig.ClusterConfigProperty#FAULT_ZONE_TYPE} for cluster config, and add new field to
+   * {@link InstanceConfig.InstanceConfigProperty#DOMAIN} that contains virtual topology group information.
+   * This is only supported for cloud environments. Cluster is expected to be in maintenance mode during config change.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#AUTO_MAINTENANCE_MODE_DISABLED} is optional.
+   *                     -- if set ture, the cluster will NOT automatically enter/exit maintenance mode during this API call;
+   *                     -- if set false or not set, the cluster will automatically enter maintenance mode and exit after
+   *                     the call succeeds. It won't proceed if the cluster is already in maintenance mode.
+   *                     Either case, the cluster must be in maintenance mode before config change.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER);
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupNumberStr), "virtualTopologyGroupNumber cannot be empty!");
+    int numGroups = 0;
+    try {
+      numGroups = Integer.parseInt(groupNumberStr);
+      Preconditions.checkArgument(numGroups > 0, "Number of virtual groups should be positive.");
+    } catch (NumberFormatException ex) {
+      throw new IllegalArgumentException("virtualTopologyGroupNumber " + groupNumberStr + " is not an integer.", ex);
+    }
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean autoMaintenanceModeDisabled = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.AUTO_MAINTENANCE_MODE_DISABLED, "false"));
+    // if auto mode is NOT disabled, let service enter maintenance mode and exit after the API succeeds.
+    if (!autoMaintenanceModeDisabled) {
+      Preconditions.checkState(!_helixAdmin.isInMaintenanceMode(clusterName),
+          "This operation is not allowed if cluster is already in maintenance mode before the API call. "
+              + "Please set autoMaintenanceModeDisabled=true if this is intended.");
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please set autoMaintenanceModeDisabled=false (default) to let the cluster enter maintenance mode automatically, "
+            + "or use autoMaintenanceModeDisabled=true and control cluster maintenance mode in client side.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (!autoMaintenanceModeDisabled) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    // update instance config
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {
+      if (!results[i]) {
+        throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));
+      }
+    }
+    // update cluster config
+    String virtualTopologyString = computeVirtualTopologyString(clusterConfig);
+    clusterConfig.setTopology(virtualTopologyString);
+    clusterConfig.setFaultZoneType(VirtualTopologyGroupConstants.VIRTUAL_FAULT_ZONE_TYPE);
+    _configAccessor.updateClusterConfig(clusterName, clusterConfig);
+    LOG.info("Successfully update instance and cluster config for {}", clusterName);
+  }
+
+  @VisibleForTesting
+  static String computeVirtualTopologyString(ClusterConfig clusterConfig) {
+    ClusterTopologyConfig clusterTopologyConfig = ClusterTopologyConfig.createFromClusterConfig(clusterConfig);
+    String endNodeType = clusterTopologyConfig.getEndNodeType();
+    String[] splits = new String[] {"", VirtualTopologyGroupConstants.VIRTUAL_FAULT_ZONE_TYPE, endNodeType};
+    return String.join(VirtualTopologyGroupConstants.PATH_NAME_SPLITTER, splits);
+  }
+
+  /**
+   * Create updater for instance config for async update.
+   * @param clusterName cluster name of the instances.
+   * @param assignment virtual group assignment.
+   * @return a map from instance zkPath to its {@link DataUpdater} to update.
+   */
+  @VisibleForTesting

Review comment:
       This annotation means the visibility of a type or member has been relaxed to make the code testable. Do you have such relaxation? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r802082302



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,194 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.

Review comment:
       Sure, updated.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on pull request #1935:
URL: https://github.com/apache/helix/pull/1935#issuecomment-1033096549


   This PR is ready to merge, approved by @zhangmeng916 
   
   Final commit message:
   
   Implement java API and utils for virtual topology group
   We introduce the java API for computing virtual topology group for cloud environments.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r786871793



##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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 static org.apache.helix.cloud.azure.AzureConstants.*;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtual_zone";

Review comment:
       Hmm, good catch. I directly followed the design, let me change it to all camelCase.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788118547



##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -52,6 +52,7 @@
     FAULT_ZONE_TYPE, // the type in which isolation should be applied on when Helix places the
     // replicas from same partition.
     TOPOLOGY_AWARE_ENABLED, // whether topology aware rebalance is enabled.
+    VIRTUAL_GROUP_ENABLED, // whether virtual topology group is enabled

Review comment:
       The idea of setting this is to avoid unexpected API call to a wrong cluster. We'd like to have this cluster config to be safer.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r798999168



##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtualZone";
+  public static final String VIRTUAL_TOPOLOGY = new StringBuilder(AzureConstants.AZURE_TOPOLOGY)

Review comment:
       Updated, this is not for a specific cloud platform, but should be universal.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r802050133



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,194 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.

Review comment:
       This doc does not convey anything other than the class name. If you do not have anywhere else document what is virtual group, you can consider putting it here, concisely. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r800891568



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#ENTER_MAINTENANCE_MODE} is optional.
+   *                     if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                     completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean enterMaintenanceMode = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.ENTER_MAINTENANCE_MODE, "true"));

Review comment:
       Good point, changed to its negation with `AUTO_MAINTENANCE_MODE_DISABLED`. 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r799911831



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.

Review comment:
       Please make sure the java doc grammatically correct.

##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestVirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.cloud.azure.AzureConstants;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.testng.Assert;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.apache.helix.cloud.constants.VirtualTopologyGroupConstants.*;
+import static org.mockito.Mockito.*;
+
+
+public class TestVirtualTopologyGroupService {
+  private static final String TEST_CLUSTER = "Test_Cluster";
+  private static final String TEST_CLUSTER0 = "TestCluster_0";
+  private static final String TEST_CLUSTER1 = "TestCluster_1";
+  private static final String TEST_CLUSTER2 = "TestCluster_2";
+
+  private final ConfigAccessor _configAccessor = mock(ConfigAccessor.class);
+  private final HelixDataAccessor _dataAccessor = mock(HelixDataAccessor.class);
+  private InstanceConfig _instanceConfig0;
+  private InstanceConfig _instanceConfig1;
+  private InstanceConfig _instanceConfig2;
+  private Map<String, DataUpdater<ZNRecord>> _updaterMap;
+  private VirtualTopologyGroupService _service;
+
+  @BeforeTest
+  public void prepare() {
+    Map<String, Set<String>> assignment = new HashMap<>();
+    _instanceConfig0 = new InstanceConfig("instance_0");
+    _instanceConfig0.setDomain("helixZoneId=zone0");
+    _instanceConfig1 = new InstanceConfig("instance_1");
+    _instanceConfig1.setDomain("helixZoneId=zone0");
+    _instanceConfig2 = new InstanceConfig("instance_2");
+    _instanceConfig2.setDomain("helixZoneId=zone1");
+
+    assignment.put("virtual_group_0", ImmutableSet.of("instance_0", "instance_1"));
+    assignment.put("virtual_group_1", ImmutableSet.of("instance_2"));
+    _updaterMap = VirtualTopologyGroupService.createInstanceConfigUpdater(TEST_CLUSTER, assignment);
+
+    ClusterConfig clusterConfig = new ClusterConfig(TEST_CLUSTER0);
+    clusterConfig.setFaultZoneType(AzureConstants.AZURE_FAULT_ZONE_TYPE);
+    clusterConfig.setTopology(AzureConstants.AZURE_TOPOLOGY);
+    clusterConfig.setTopologyAwareEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER0)).thenReturn(clusterConfig);
+
+    CloudConfig.Builder cloudConfigBuilder = new CloudConfig.Builder();
+    cloudConfigBuilder.setCloudEnabled(true);
+    cloudConfigBuilder.setCloudProvider(CloudProvider.AZURE);
+    cloudConfigBuilder.setCloudID("TestID");
+    CloudConfig cloudConfig = cloudConfigBuilder.build();
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER0)).thenReturn(cloudConfig);
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER2)).thenReturn(cloudConfig);
+
+    HelixAdmin helixAdmin = mock(HelixAdmin.class);
+    when(helixAdmin.isInMaintenanceMode(anyString())).thenReturn(true);
+
+    boolean[] results = new boolean[2];
+    results[0] = results[1] = true;
+    when(_dataAccessor.updateChildren(anyList(), anyList(), anyInt())).thenReturn(results);
+    ClusterService clusterService = mock(ClusterService.class);
+    when(clusterService.getClusterTopology(anyString())).thenReturn(prepareClusterTopology());
+    _service = new VirtualTopologyGroupService(helixAdmin, clusterService, _configAccessor, _dataAccessor);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Cloud is not enabled.*")
+  public void testClusterCloudConfigSetup() {
+    ClusterConfig clusterConfig1 = new ClusterConfig(TEST_CLUSTER1);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER1)).thenReturn(clusterConfig1);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER1, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"));
+  }
+
+  @Test
+  public void testVirtualTopologyGroupService() {
+    _service.addVirtualTopologyGroup(TEST_CLUSTER0, ImmutableMap.of(
+        GROUP_NAME, "test-group", GROUP_NUMBER, "2", ENTER_MAINTENANCE_MODE, "false"));
+    verify(_dataAccessor, times(1)).updateChildren(anyList(), anyList(), anyInt());
+    verify(_configAccessor, times(1)).updateClusterConfig(anyString(), any());
+  }
+
+  @Test(expectedExceptions = IllegalStateException.class,
+      expectedExceptionsMessageRegExp = "This operation is not allowed while cluster is in maintenance mode.*")

Review comment:
       Is the exception correct? Should be "the operation is not allowed while cluster is not in maintenance mode"?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#ENTER_MAINTENANCE_MODE} is optional.
+   *                     if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                     completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean enterMaintenanceMode = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.ENTER_MAINTENANCE_MODE, "true"));

Review comment:
       What are we trying to achieve with this extra config `ENTER_MAINTENANCE_MODE`? Are we trying to avoid the case where customers call this API but they do not want to enter maintenance mode? Then the call will fail, right? This means customers use this API in a wrong way. Please let me know if you have other intention with this config.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r786869819



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.controller.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {

Review comment:
       The reason I chose the current way is to print the failed instance address, it's the first failed one though. Let me update it in the next revision. 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788110306



##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtualZone";
+  public static final String VIRTUAL_TOPOLOGY = new StringBuilder(AzureConstants.AZURE_TOPOLOGY)

Review comment:
       It's only used in Azure, but not always, and never 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] narendly commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r786368109



##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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 static org.apache.helix.cloud.azure.AzureConstants.*;

Review comment:
       Note on style: generally we avoid static wildcard imports

##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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 static org.apache.helix.cloud.azure.AzureConstants.*;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtual_zone";

Review comment:
       Is there a reason we're not being consistent with casing? Two are camelCase and one is underscored?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.controller;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       Usually Java Enums are used to define values where Java Interfaces are used to define behavior (or the default behavior and make it extendible). Is there a reason we're making this an Enum here?
   
   What I would suggest is to create a separate package (I'm not sure this belongs to `org.apache.helix.controller` - maybe something like `*.cloud.groupingStrategy` or `*.topology.groupingStrategy` might be more appropriate?) and define an interface with a default implementation?

##########
File path: helix-core/src/main/java/org/apache/helix/controller/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.controller;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable whereas:

Review comment:
       nit: "whereas" -> "where"




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r791063716



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.

Review comment:
       Thanks for the comment, the assignment algorithm will be revised and validated with more clear metrics and benchmarks. There will be a design review as well. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r791063716



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.

Review comment:
       Thanks for the comment, the assignment algorithm will be revised and validated with more clear metrics and benchmarks. There will be a design review as well. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r799000168



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {

Review comment:
       Updated, we are discarding this config.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r799923957



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#ENTER_MAINTENANCE_MODE} is optional.
+   *                     if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                     completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean enterMaintenanceMode = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.ENTER_MAINTENANCE_MODE, "true"));

Review comment:
       No, we ensure the cluster is in maintenance mode before config change in either case.
   When this is enabled, the service will enter and exit maintenance mode during the API call for safe operation. 
   We also expect users may want more control and don’t want the service enter/exit maintenance mode automatically. So when it’s disabled, the service won’t change the maintenance mode, but will still check it before config change.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r802076381



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,194 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.cloud.constants.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.FifoVirtualGroupAssignmentAlgorithm;
+import org.apache.helix.cloud.topology.VirtualGroupAssignmentAlgorithm;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.ClusterTopologyConfig;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private static final Logger LOG = LoggerFactory.getLogger(VirtualTopologyGroupService.class);
+
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. We override {@link ClusterConfig.ClusterConfigProperty#TOPOLOGY} and
+   * {@link ClusterConfig.ClusterConfigProperty#FAULT_ZONE_TYPE} for cluster config, and add new field to
+   * {@link InstanceConfig.InstanceConfigProperty#DOMAIN} that contains virtual topology group information.
+   * This is only supported for cloud environments. Cluster is expected to be in maintenance mode during config change.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required,
+   *                     {@link VirtualTopologyGroupConstants#AUTO_MAINTENANCE_MODE_DISABLED} is optional.
+   *                     -- if set ture, the cluster will NOT automatically enter/exit maintenance mode during this API call;
+   *                     -- if set false or not set, the cluster will automatically enter maintenance mode and exit after
+   *                     the call succeeds. It won't proceed if the cluster is already in maintenance mode.
+   *                     Either case, the cluster must be in maintenance mode before config change.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields) {
+    // validation
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    Preconditions.checkState(clusterConfig.isTopologyAwareEnabled(),
+        "Topology-aware rebalance is not enabled in cluster " + clusterName);
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER);
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    Preconditions.checkArgument(!StringUtils.isEmpty(groupNumberStr), "virtualTopologyGroupNumber cannot be empty!");
+    int numGroups = 0;
+    try {
+      numGroups = Integer.parseInt(groupNumberStr);
+      Preconditions.checkArgument(numGroups > 0, "Number of virtual groups should be positive.");
+    } catch (NumberFormatException ex) {
+      throw new IllegalArgumentException("virtualTopologyGroupNumber " + groupNumberStr + " is not an integer.", ex);
+    }
+    LOG.info("Computing virtual topology group for cluster {} with param {}", clusterName, customFields);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Preconditions.checkArgument(numGroups <= clusterTopology.getAllInstances().size(),
+        "Number of virtual groups cannot be greater than the number of instances.");
+    Map<String, Set<String>> assignment =
+        _assignmentAlgorithm.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    boolean autoMaintenanceModeDisabled = Boolean.parseBoolean(
+        customFields.getOrDefault(VirtualTopologyGroupConstants.AUTO_MAINTENANCE_MODE_DISABLED, "false"));
+    // if auto mode is NOT disabled, let service enter maintenance mode and exit after the API succeeds.
+    if (!autoMaintenanceModeDisabled) {
+      Preconditions.checkState(!_helixAdmin.isInMaintenanceMode(clusterName),
+          "This operation is not allowed if cluster is already in maintenance mode before the API call. "
+              + "Please set autoMaintenanceModeDisabled=true if this is intended.");
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please set autoMaintenanceModeDisabled=false (default) to let the cluster enter maintenance mode automatically, "
+            + "or use autoMaintenanceModeDisabled=true and control cluster maintenance mode in client side.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (!autoMaintenanceModeDisabled) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    // update instance config
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {
+      if (!results[i]) {
+        throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));
+      }
+    }
+    // update cluster config
+    String virtualTopologyString = computeVirtualTopologyString(clusterConfig);
+    clusterConfig.setTopology(virtualTopologyString);
+    clusterConfig.setFaultZoneType(VirtualTopologyGroupConstants.VIRTUAL_FAULT_ZONE_TYPE);
+    _configAccessor.updateClusterConfig(clusterName, clusterConfig);
+    LOG.info("Successfully update instance and cluster config for {}", clusterName);
+  }
+
+  @VisibleForTesting
+  static String computeVirtualTopologyString(ClusterConfig clusterConfig) {
+    ClusterTopologyConfig clusterTopologyConfig = ClusterTopologyConfig.createFromClusterConfig(clusterConfig);
+    String endNodeType = clusterTopologyConfig.getEndNodeType();
+    String[] splits = new String[] {"", VirtualTopologyGroupConstants.VIRTUAL_FAULT_ZONE_TYPE, endNodeType};
+    return String.join(VirtualTopologyGroupConstants.PATH_NAME_SPLITTER, splits);
+  }
+
+  /**
+   * Create updater for instance config for async update.
+   * @param clusterName cluster name of the instances.
+   * @param assignment virtual group assignment.
+   * @return a map from instance zkPath to its {@link DataUpdater} to update.
+   */
+  @VisibleForTesting

Review comment:
       Yes, it's relaxed from private in order for testing, see `TestVirtualTopologyGroupService`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r786916212



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.controller.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {

Review comment:
       To update, I just realized we can't stream on `boolean[]`, it has to be `Boolean[]` It seems too much effort and potential performance drop to make this work. If you don't have strong opinion on this, I'll stick with the original. 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r791024954



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.

Review comment:
       What do we compare and conclude the "instance shuffles is reduced"? Could you please illustrate? Is this the best strategy for deterministic choices? 

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {

Review comment:
       Actually I don't see how adding this config could improve safety. As long as cloud config is on, we allow virtual grouping, at least this is the agreement for now. Whether the virtual grouping is used correctly is purely decided by users' knowledge. Asking users to turn on another config does not really help ensure the correctness.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {
+      if (!results[i]) {
+        throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));

Review comment:
       So this fails, we have configs partially updated. Will this impact controller's pipeline logic before we update them again? Please provide some insights.

##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;

Review comment:
       +1. Why virtual topology should be bound with Azure? It should not have anything dependent on a specific type of cloud environment. Before using virtual grouping, you only need cloud config is on, but you don't need the topology to be the Azure one. 

##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestVirtualTopologyGroupService.java
##########
@@ -0,0 +1,172 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.cloud.azure.AzureConstants;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.testng.Assert;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.apache.helix.common.VirtualTopologyGroupConstants.*;
+import static org.mockito.Mockito.*;
+
+
+public class TestVirtualTopologyGroupService {
+  private static final String TEST_CLUSTER = "Test_Cluster";
+  private static final String TEST_CLUSTER0 = "TestCluster_0";
+  private static final String TEST_CLUSTER1 = "TestCluster_1";
+  private static final String TEST_CLUSTER2 = "TestCluster_2";
+
+  private final ConfigAccessor _configAccessor = mock(ConfigAccessor.class);
+  private final HelixDataAccessor _dataAccessor = mock(HelixDataAccessor.class);
+  private InstanceConfig _instanceConfig0;
+  private InstanceConfig _instanceConfig1;
+  private InstanceConfig _instanceConfig2;
+  private Map<String, DataUpdater<ZNRecord>> _updaterMap;
+  private VirtualTopologyGroupService _service;
+
+  @BeforeTest
+  public void prepare() {
+    Map<String, Set<String>> assignment = new HashMap<>();
+    _instanceConfig0 = new InstanceConfig("instance_0");
+    _instanceConfig0.setDomain("helixZoneId=zone0");
+    _instanceConfig1 = new InstanceConfig("instance_1");
+    _instanceConfig1.setDomain("helixZoneId=zone0");
+    _instanceConfig2 = new InstanceConfig("instance_2");
+    _instanceConfig2.setDomain("helixZoneId=zone1");
+
+    assignment.put("virtual_group_0", ImmutableSet.of("instance_0", "instance_1"));
+    assignment.put("virtual_group_1", ImmutableSet.of("instance_2"));
+    _updaterMap = VirtualTopologyGroupService.createInstanceConfigUpdater(TEST_CLUSTER, assignment);
+
+    ClusterConfig clusterConfig = new ClusterConfig(TEST_CLUSTER0);
+    clusterConfig.setFaultZoneType(AzureConstants.AZURE_FAULT_ZONE_TYPE);
+    clusterConfig.setTopology(AzureConstants.AZURE_TOPOLOGY);
+    clusterConfig.setVirtualGroupEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER0)).thenReturn(clusterConfig);
+
+    CloudConfig.Builder cloudConfigBuilder = new CloudConfig.Builder();
+    cloudConfigBuilder.setCloudEnabled(true);
+    cloudConfigBuilder.setCloudProvider(CloudProvider.AZURE);
+    cloudConfigBuilder.setCloudID("TestID");
+    CloudConfig cloudConfig = cloudConfigBuilder.build();
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER0)).thenReturn(cloudConfig);
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER2)).thenReturn(cloudConfig);
+
+    HelixAdmin helixAdmin = mock(HelixAdmin.class);
+    when(helixAdmin.isInMaintenanceMode(anyString())).thenReturn(true);
+
+    boolean[] results = new boolean[2];
+    results[0] = results[1] = true;
+    when(_dataAccessor.updateChildren(anyList(), anyList(), anyInt())).thenReturn(results);
+    ClusterService clusterService = mock(ClusterService.class);
+    when(clusterService.getClusterTopology(anyString())).thenReturn(prepareClusterTopology());
+    _service = new VirtualTopologyGroupService(helixAdmin, clusterService, _configAccessor, _dataAccessor);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Cloud is not enabled.*")
+  public void testClusterCloudConfigSetup() {
+    ClusterConfig clusterConfig1 = new ClusterConfig(TEST_CLUSTER1);
+    clusterConfig1.setVirtualGroupEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER1)).thenReturn(clusterConfig1);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER1, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"), true);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Virtual Group is disabled in cluster.*")
+  public void testClusterVirtualGroupConfigSetup() {
+    ClusterConfig clusterConfig2 = new ClusterConfig(TEST_CLUSTER2);
+    clusterConfig2.setVirtualGroupEnabled(false);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER2)).thenReturn(clusterConfig2);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER2, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"), true);
+  }
+
+  @Test
+  public void testService() {

Review comment:
       Please have more concrete naming for tests. This is difficult to tell what is being tested, especially if it's in the log.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788116280



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {

Review comment:
       >  then your disabling maintenance mode in line 111 will remove user's previous setup
   
   How so? If `enterMaintenanceMode` is false, we rely on user setup and won't change the maintenance mode.
   Are you saying for the case when `enterMaintenanceMode` is true, we shouldn't proceed if maintenance mode is already on?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r800893000



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestVirtualTopologyGroupService.java
##########
@@ -0,0 +1,183 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.cloud.azure.AzureConstants;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.testng.Assert;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.apache.helix.cloud.constants.VirtualTopologyGroupConstants.*;
+import static org.mockito.Mockito.*;
+
+
+public class TestVirtualTopologyGroupService {
+  private static final String TEST_CLUSTER = "Test_Cluster";
+  private static final String TEST_CLUSTER0 = "TestCluster_0";
+  private static final String TEST_CLUSTER1 = "TestCluster_1";
+  private static final String TEST_CLUSTER2 = "TestCluster_2";
+
+  private final ConfigAccessor _configAccessor = mock(ConfigAccessor.class);
+  private final HelixDataAccessor _dataAccessor = mock(HelixDataAccessor.class);
+  private InstanceConfig _instanceConfig0;
+  private InstanceConfig _instanceConfig1;
+  private InstanceConfig _instanceConfig2;
+  private Map<String, DataUpdater<ZNRecord>> _updaterMap;
+  private VirtualTopologyGroupService _service;
+
+  @BeforeTest
+  public void prepare() {
+    Map<String, Set<String>> assignment = new HashMap<>();
+    _instanceConfig0 = new InstanceConfig("instance_0");
+    _instanceConfig0.setDomain("helixZoneId=zone0");
+    _instanceConfig1 = new InstanceConfig("instance_1");
+    _instanceConfig1.setDomain("helixZoneId=zone0");
+    _instanceConfig2 = new InstanceConfig("instance_2");
+    _instanceConfig2.setDomain("helixZoneId=zone1");
+
+    assignment.put("virtual_group_0", ImmutableSet.of("instance_0", "instance_1"));
+    assignment.put("virtual_group_1", ImmutableSet.of("instance_2"));
+    _updaterMap = VirtualTopologyGroupService.createInstanceConfigUpdater(TEST_CLUSTER, assignment);
+
+    ClusterConfig clusterConfig = new ClusterConfig(TEST_CLUSTER0);
+    clusterConfig.setFaultZoneType(AzureConstants.AZURE_FAULT_ZONE_TYPE);
+    clusterConfig.setTopology(AzureConstants.AZURE_TOPOLOGY);
+    clusterConfig.setTopologyAwareEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER0)).thenReturn(clusterConfig);
+
+    CloudConfig.Builder cloudConfigBuilder = new CloudConfig.Builder();
+    cloudConfigBuilder.setCloudEnabled(true);
+    cloudConfigBuilder.setCloudProvider(CloudProvider.AZURE);
+    cloudConfigBuilder.setCloudID("TestID");
+    CloudConfig cloudConfig = cloudConfigBuilder.build();
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER0)).thenReturn(cloudConfig);
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER2)).thenReturn(cloudConfig);
+
+    HelixAdmin helixAdmin = mock(HelixAdmin.class);
+    when(helixAdmin.isInMaintenanceMode(anyString())).thenReturn(true);
+
+    boolean[] results = new boolean[2];
+    results[0] = results[1] = true;
+    when(_dataAccessor.updateChildren(anyList(), anyList(), anyInt())).thenReturn(results);
+    ClusterService clusterService = mock(ClusterService.class);
+    when(clusterService.getClusterTopology(anyString())).thenReturn(prepareClusterTopology());
+    _service = new VirtualTopologyGroupService(helixAdmin, clusterService, _configAccessor, _dataAccessor);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Cloud is not enabled.*")
+  public void testClusterCloudConfigSetup() {
+    ClusterConfig clusterConfig1 = new ClusterConfig(TEST_CLUSTER1);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER1)).thenReturn(clusterConfig1);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER1, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"));
+  }
+
+  @Test
+  public void testVirtualTopologyGroupService() {
+    _service.addVirtualTopologyGroup(TEST_CLUSTER0, ImmutableMap.of(
+        GROUP_NAME, "test-group", GROUP_NUMBER, "2", ENTER_MAINTENANCE_MODE, "false"));
+    verify(_dataAccessor, times(1)).updateChildren(anyList(), anyList(), anyInt());
+    verify(_configAccessor, times(1)).updateClusterConfig(anyString(), any());
+  }
+
+  @Test(expectedExceptions = IllegalStateException.class,
+      expectedExceptionsMessageRegExp = "This operation is not allowed while cluster is in maintenance mode.*")

Review comment:
       Same as above comment, it is expected. The cluster shouldn't be already in maintenance mode if in auto mode. Updated the config name and exception message to make it more clear.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r791024954



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.

Review comment:
       What do we compare and conclude the "instance shuffles is reduced"? Could you please illustrate? Is this the best strategy for deterministic choices? 

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {

Review comment:
       Actually I don't see how adding this config could improve safety. As long as cloud config is on, we allow virtual grouping, at least this is the agreement for now. Whether the virtual grouping is used correctly is purely decided by users' knowledge. Asking users to turn on another config does not really help ensure the correctness.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {
+      if (!results[i]) {
+        throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));

Review comment:
       So this fails, we have configs partially updated. Will this impact controller's pipeline logic before we update them again? Please provide some insights.

##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;

Review comment:
       +1. Why virtual topology should be bound with Azure? It should not have anything dependent on a specific type of cloud environment. Before using virtual grouping, you only need cloud config is on, but you don't need the topology to be the Azure one. 

##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestVirtualTopologyGroupService.java
##########
@@ -0,0 +1,172 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.cloud.azure.AzureConstants;
+import org.apache.helix.cloud.constants.CloudProvider;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+import org.testng.Assert;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.apache.helix.common.VirtualTopologyGroupConstants.*;
+import static org.mockito.Mockito.*;
+
+
+public class TestVirtualTopologyGroupService {
+  private static final String TEST_CLUSTER = "Test_Cluster";
+  private static final String TEST_CLUSTER0 = "TestCluster_0";
+  private static final String TEST_CLUSTER1 = "TestCluster_1";
+  private static final String TEST_CLUSTER2 = "TestCluster_2";
+
+  private final ConfigAccessor _configAccessor = mock(ConfigAccessor.class);
+  private final HelixDataAccessor _dataAccessor = mock(HelixDataAccessor.class);
+  private InstanceConfig _instanceConfig0;
+  private InstanceConfig _instanceConfig1;
+  private InstanceConfig _instanceConfig2;
+  private Map<String, DataUpdater<ZNRecord>> _updaterMap;
+  private VirtualTopologyGroupService _service;
+
+  @BeforeTest
+  public void prepare() {
+    Map<String, Set<String>> assignment = new HashMap<>();
+    _instanceConfig0 = new InstanceConfig("instance_0");
+    _instanceConfig0.setDomain("helixZoneId=zone0");
+    _instanceConfig1 = new InstanceConfig("instance_1");
+    _instanceConfig1.setDomain("helixZoneId=zone0");
+    _instanceConfig2 = new InstanceConfig("instance_2");
+    _instanceConfig2.setDomain("helixZoneId=zone1");
+
+    assignment.put("virtual_group_0", ImmutableSet.of("instance_0", "instance_1"));
+    assignment.put("virtual_group_1", ImmutableSet.of("instance_2"));
+    _updaterMap = VirtualTopologyGroupService.createInstanceConfigUpdater(TEST_CLUSTER, assignment);
+
+    ClusterConfig clusterConfig = new ClusterConfig(TEST_CLUSTER0);
+    clusterConfig.setFaultZoneType(AzureConstants.AZURE_FAULT_ZONE_TYPE);
+    clusterConfig.setTopology(AzureConstants.AZURE_TOPOLOGY);
+    clusterConfig.setVirtualGroupEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER0)).thenReturn(clusterConfig);
+
+    CloudConfig.Builder cloudConfigBuilder = new CloudConfig.Builder();
+    cloudConfigBuilder.setCloudEnabled(true);
+    cloudConfigBuilder.setCloudProvider(CloudProvider.AZURE);
+    cloudConfigBuilder.setCloudID("TestID");
+    CloudConfig cloudConfig = cloudConfigBuilder.build();
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER0)).thenReturn(cloudConfig);
+    when(_configAccessor.getCloudConfig(TEST_CLUSTER2)).thenReturn(cloudConfig);
+
+    HelixAdmin helixAdmin = mock(HelixAdmin.class);
+    when(helixAdmin.isInMaintenanceMode(anyString())).thenReturn(true);
+
+    boolean[] results = new boolean[2];
+    results[0] = results[1] = true;
+    when(_dataAccessor.updateChildren(anyList(), anyList(), anyInt())).thenReturn(results);
+    ClusterService clusterService = mock(ClusterService.class);
+    when(clusterService.getClusterTopology(anyString())).thenReturn(prepareClusterTopology());
+    _service = new VirtualTopologyGroupService(helixAdmin, clusterService, _configAccessor, _dataAccessor);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Cloud is not enabled.*")
+  public void testClusterCloudConfigSetup() {
+    ClusterConfig clusterConfig1 = new ClusterConfig(TEST_CLUSTER1);
+    clusterConfig1.setVirtualGroupEnabled(true);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER1)).thenReturn(clusterConfig1);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER1, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"), true);
+  }
+
+  @Test(expectedExceptions = HelixException.class, expectedExceptionsMessageRegExp = "Virtual Group is disabled in cluster.*")
+  public void testClusterVirtualGroupConfigSetup() {
+    ClusterConfig clusterConfig2 = new ClusterConfig(TEST_CLUSTER2);
+    clusterConfig2.setVirtualGroupEnabled(false);
+    when(_configAccessor.getClusterConfig(TEST_CLUSTER2)).thenReturn(clusterConfig2);
+    _service.addVirtualTopologyGroup(
+        TEST_CLUSTER2, ImmutableMap.of(GROUP_NAME, "test-group", GROUP_NUMBER, "2"), true);
+  }
+
+  @Test
+  public void testService() {

Review comment:
       Please have more concrete naming for tests. This is difficult to tell what is being tested, especially if it's in the log.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r799001550



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, true,
+          "Enable maintenanceMode for virtual topology group change.", customFields);
+    }
+    Preconditions.checkState(_helixAdmin.isInMaintenanceMode(clusterName),
+        "Cluster is not in maintenance mode. This is required for virtual topology group setting. "
+            + "Please enable enterMaintenanceMode or enter maintenance mode for the cluster prior to the API call.");
+
+    updateConfigs(clusterName, clusterConfig, assignment);
+    if (enterMaintenanceMode) {
+      _helixAdmin.manuallyEnableMaintenanceMode(clusterName, false,
+          "Disable maintenanceMode after virtual topology group change.", customFields);
+    }
+  }
+
+  private void updateConfigs(String clusterName, ClusterConfig clusterConfig, Map<String, Set<String>> assignment) {
+    List<String> zkPaths = new ArrayList<>();
+    List<DataUpdater<ZNRecord>> updaters = new ArrayList<>();
+    createInstanceConfigUpdater(clusterName, assignment).forEach((zkPath, updater) -> {
+      zkPaths.add(zkPath);
+      updaters.add(updater);
+    });
+    boolean[] results = _dataAccessor.updateChildren(zkPaths, updaters, AccessOption.EPHEMERAL);
+    for (int i = 0; i < results.length; i++) {
+      if (!results[i]) {
+        throw new HelixException("Failed to update instance config for path " + zkPaths.get(i));

Review comment:
       We ensure the cluster is in maintenance mode during config change. So if it fails, we expect human intervention, meanwhile the cluster stays in maintenance mode to prevent any partition movement.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r786903019



##########
File path: helix-core/src/main/java/org/apache/helix/controller/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.controller;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       Thanks for the comment, it's really good question! I hesitated a lot in choosing enum over interface, here are my considerations:
   1. Enum defines constants that could go with predefined behavior (enum methods), and strategy falls in the category. Plus enum is naturally static final singleton, and it's compile-time constant, method is also resolved at compile time, while interface is not.
   2. Afaik, we don't have dependency injection to decouple interface and impl, instead we directly construct impl class. I would try to avoid creating interface if there is only one known impl.
   3. The key method is static. With interface we could use a static or default method, but it's weird to instantiate a class for static method. 
   4. Writing logic in a util class also works, while enum provides a layer of encapsulation and is type safe.
   
   This is open discussion, using interface here won't be too bad after all. Let me know how you feel about it. Thanks again.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] jiajunwang commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788067380



##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;
+
+
+public class VirtualTopologyGroupConstants {
+  public static final String GROUP_NAME = "virtualTopologyGroupName";
+  public static final String GROUP_NUMBER = "virtualTopologyGroupNumber";
+  public static final String VIRTUAL_FAULT_ZONE_TYPE = "virtualZone";
+  public static final String VIRTUAL_TOPOLOGY = new StringBuilder(AzureConstants.AZURE_TOPOLOGY)

Review comment:
       Does it imply this feature is always used in Azure? I don't think so.
   Maybe move this final joiner logic to the service?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");

Review comment:
       Why is this required? As I discussed with Quincy, I think part of the purpose we think adding the virtual group is a good idea is because users may want more replicas than FZ count for scaling workload. If this is a target, then it should be an orthogonal feature than cloud support. 

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);

Review comment:
       Validate numGroups too?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED

Review comment:
       The fact that users call addVirtualTopologyGroup implies that they want to enable VIRTUAL_GROUP_ENABLED. I think VIRTUAL_GROUP_ENABLED is redundant.

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       No strong preference, but I prefer to avoid using "strategy", which immediately hint to the code reader/lib user about partition rebalance strategy.

##########
File path: helix-core/src/main/java/org/apache/helix/common/VirtualTopologyGroupConstants.java
##########
@@ -0,0 +1,31 @@
+package org.apache.helix.common;
+
+/*
+ * 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.cloud.azure.AzureConstants;

Review comment:
       Should we put it in the same cloud package? Or maybe we should remove the cloud-specific items here so it is more "common".

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {
+
+  /**
+   * A default assignment strategy that is deterministic and stable where:
+   * 1. assignment is guaranteed consistent for same inputs.
+   * 2. number of instance shuffles is reduced.
+   */
+  DEFAULT {
+    @Override
+    public Map<String, Set<String>> computeAssignment(
+        int numGroups, String virtualGroupName, Map<String, Set<String>> zoneMapping) {
+      List<String> sortedInstances = HelixUtil.sortAndFlattenZoneMapping(zoneMapping);
+      Map<String, Set<String>> assignment = new HashMap<>();
+      int instancesPerGroup = sortedInstances.size() / numGroups;

Review comment:
       Would it be easier if you take the ceiling of the float number and do the assignment based on that? Then we don't need step 2.

##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -52,6 +52,7 @@
     FAULT_ZONE_TYPE, // the type in which isolation should be applied on when Helix places the
     // replicas from same partition.
     TOPOLOGY_AWARE_ENABLED, // whether topology aware rebalance is enabled.
+    VIRTUAL_GROUP_ENABLED, // whether virtual topology group is enabled

Review comment:
       Why do we need this? At least in stage one, while everyone is using the API to generate virtual groups at the very beginning, there is no need to check if it is enabled or not, right? Please correct me if my understanding is wrong.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);
+
+    // compute group assignment
+    ClusterTopology clusterTopology = _clusterService.getClusterTopology(clusterName);
+    Map<String, Set<String>> assignment =
+        _strategy.computeAssignment(numGroups, groupName, clusterTopology.toZoneMapping());
+
+    if (enterMaintenanceMode) {

Review comment:
       Somehow I believe you can call manuallyEnableMaintenanceMode even the cluster is already in the maintenance mode. If this is the case, then your disabling maintenance mode in line 111 will remove user's previous setup. And it could cause serious issues.
   Could you please confirm? We should either fail the manuallyEnableMaintenanceMode if the cluster is in maintenance mode, or don't disabling if the maintenance mode is not triggered by our API logic.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;

Review comment:
       Are we planning to support multiple strategies in the future?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788132875



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED
+    CloudConfig cloudConfig = _configAccessor.getCloudConfig(clusterName);
+    if (cloudConfig == null || !cloudConfig.isCloudEnabled()) {
+      throw new HelixException(
+          "Cloud is not enabled, addVirtualTopologyGroup is not allowed to run in non-cloud environment.");
+    }
+    ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName);
+    if (!clusterConfig.isVirtualGroupEnabled()) {
+      throw new HelixException("Virtual Group is disabled in cluster " + clusterName);
+    }
+    // validation
+    String groupName = customFields.get(VirtualTopologyGroupConstants.GROUP_NAME);
+    String groupNumberStr = Preconditions.checkNotNull(
+        customFields.get(VirtualTopologyGroupConstants.GROUP_NUMBER),
+        "virtualTopologyGroupNumber cannot be empty!");
+    Preconditions.checkState(!StringUtils.isEmpty(groupName), "virtualTopologyGroupName cannot be empty!");
+    int numGroups = Integer.parseInt(groupNumberStr);

Review comment:
       We have null check on `groupNumberStr` already. 
   Will simply verify it's a positive integer, I think it's fine to just let it fail at parsing (handling that `NumberFormatException` seems redundant)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788124546



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.cloud.topology;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       I agree, if there is other strategy out there might be confusing. Do you have a naming proposal? Algorithm?
   "strategy" still sounds fit the most to me in this use 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] narendly commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r787230750



##########
File path: helix-core/src/main/java/org/apache/helix/controller/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.controller;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       Could `VirtualTopologyGroupStrategy` have multiple implementations and do we expect to add different implementations and make this configurable? If so, I think using an abstract base class could also work.
   
   Or is the goal to provide a static method? if so i think a util class would fit better here. Since this is only used in `VirtualTopologyGroupService` and only fixed to a `DEFAULT` implementation. Is this class only going to be used in Helix REST? If that's the case, I'd place this in that module.
   
   I'm confused because I'm not sure what the intent of this class is - it also has an abstract method and the default enum is a behavior (verb) than named variables (nouns) that enums generally are.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r787290107



##########
File path: helix-core/src/main/java/org/apache/helix/controller/VirtualTopologyGroupStrategy.java
##########
@@ -0,0 +1,80 @@
+package org.apache.helix.controller;
+
+/*
+ * 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 java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.helix.util.HelixUtil;
+
+public enum VirtualTopologyGroupStrategy {

Review comment:
       The Enum `VirtualTopologyGroupStrategy` may have other enum values, e.g. `STRATEGY1`, `STRATEGY2` (in parallel to `DEFAULT` , they can have their own method implementation. And it can be configurable which one to use by parsing string.
   To clarify, the reason I created this class is to have a way to "implement" static method as in interface. So each enu value defines a strategy, while each strategy comes with an assignment algorithm (abstract method). Indeed, a util class fits here, but I chose to use Enum for better encapsulation. FYI, https://blogs.oracle.com/javamagazine/post/how-to-make-the-most-of-java-enums. 
   That said, I'm afraid this is creating more confusion than I expected, I'll change it to a util class if you don't have objection. We can revisit this when we want to add other implementations. 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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] qqu0127 commented on a change in pull request #1935: Implement java API and utils for virtual topology group

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1935:
URL: https://github.com/apache/helix/pull/1935#discussion_r788119371



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
##########
@@ -0,0 +1,163 @@
+package org.apache.helix.rest.server.service;
+
+/*
+ * 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.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.HelixException;
+import org.apache.helix.common.VirtualTopologyGroupConstants;
+import org.apache.helix.cloud.topology.VirtualTopologyGroupStrategy;
+import org.apache.helix.model.CloudConfig;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.builder.HelixConfigScopeBuilder;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.DataUpdater;
+
+
+/**
+ * Service for virtual topology group.
+ */
+public class VirtualTopologyGroupService {
+  private final HelixAdmin _helixAdmin;
+  private final ClusterService _clusterService;
+  private final ConfigAccessor _configAccessor;
+  private final HelixDataAccessor _dataAccessor;
+  private final VirtualTopologyGroupStrategy _strategy;
+
+  public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService clusterService,
+      ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
+    _helixAdmin = helixAdmin;
+    _clusterService = clusterService;
+    _configAccessor = configAccessor;
+    _dataAccessor = dataAccessor;
+    _strategy = VirtualTopologyGroupStrategy.DEFAULT;
+  }
+
+  /**
+   * Add virtual topology group for a cluster.
+   * This includes calculating the virtual group assignment for all instances in the cluster then update instance config
+   * and cluster config. Cluster is expected to enter maintenanceMode during config update, this is either enabled/disabled
+   * in place this method or handled by client side code.
+   * @param clusterName the cluster name.
+   * @param customFields custom fields, {@link VirtualTopologyGroupConstants#GROUP_NAME}
+   *                     and {@link VirtualTopologyGroupConstants#GROUP_NUMBER} are required
+   * @param enterMaintenanceMode if enabled, the cluster will enter maintenance mode during the setup and exit once it
+   *                             completes. Otherwise, it's expected the maintenanceMode is controlled by client side.
+   */
+  public void addVirtualTopologyGroup(String clusterName, Map<String, String> customFields, boolean enterMaintenanceMode) {
+    // only support if CLOUD_ENABLED AND VIRTUAL_GROUP_ENABLED

Review comment:
       Same as above.
   I don't have strong opinion on this though, we can discuss more during the meeting.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org