narendly commented on a change in pull request #1031:
URL: https://github.com/apache/helix/pull/1031#discussion_r431456337
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/ReadOnlyWagedRebalancer.java
##########
@@ -0,0 +1,88 @@
+package org.apache.helix.controller.rebalancer.waged;
Review comment:
I'm not sure if that's a good idea. I've considered that option and that
might even be more confusing.
Note that users do not use rebalancers directly anyway. So I think this
would be the appropriate package to put it in along with the original
WagedRebalancer.
##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -140,6 +160,81 @@ public static String serializeByComma(List<String>
objects) {
}
}
+ /**
+ * Returns the expected ideal ResourceAssignments for the given resources in
the cluster
+ * calculated using the read-only WAGED rebalancer.
+ * @param metadataStoreAddress
+ * @param clusterConfig
+ * @param instanceConfigs
+ * @param liveInstances
+ * @param newIdealStates
+ * @param newResourceConfigs
+ * @return
+ */
+ public static Map<String, ResourceAssignment>
getIdealAssignmentForWagedFullAuto(
+ String metadataStoreAddress, ClusterConfig clusterConfig,
+ List<InstanceConfig> instanceConfigs, List<String> liveInstances,
+ List<IdealState> newIdealStates, List<ResourceConfig>
newResourceConfigs) {
+ // Prepare a data accessor for a dataProvider (cache) refresh
+ BaseDataAccessor<ZNRecord> baseDataAccessor = new
ZkBaseDataAccessor<>(metadataStoreAddress);
+ HelixDataAccessor helixDataAccessor =
+ new ZKHelixDataAccessor(clusterConfig.getClusterName(),
baseDataAccessor);
+
+ // Obtain a refreshed dataProvider (cache) and overwrite cluster
parameters with the given parameters
+ ResourceControllerDataProvider dataProvider =
+ new ResourceControllerDataProvider(clusterConfig.getClusterName());
+ dataProvider.requireFullRefresh();
+ dataProvider.refresh(helixDataAccessor);
+ dataProvider.setClusterConfig(clusterConfig);
+ dataProvider.setInstanceConfigMap(instanceConfigs.stream()
+ .collect(Collectors.toMap(InstanceConfig::getInstanceName,
Function.identity())));
+ dataProvider.setLiveInstances(
+
liveInstances.stream().map(LiveInstance::new).collect(Collectors.toList()));
+ dataProvider.setIdealStates(newIdealStates);
+ dataProvider.setResourceConfigMap(newResourceConfigs.stream()
+ .collect(Collectors.toMap(ResourceConfig::getResourceName,
Function.identity())));
+
+ // Create an instance of read-only WAGED rebalancer
+ ReadOnlyWagedRebalancer readOnlyWagedRebalancer =
+ new ReadOnlyWagedRebalancer(metadataStoreAddress,
clusterConfig.getClusterName(),
+ clusterConfig.getGlobalRebalancePreference());
+
+ // Use a dummy event to run the required stages for BestPossibleState
calculation
+ // Attributes RESOURCES and RESOURCES_TO_REBALANCE are populated in
ResourceComputationStage
+ ClusterEvent event = new ClusterEvent(clusterConfig.getClusterName(),
ClusterEventType.Unknown);
+ event.addAttribute(AttributeName.ControllerDataProvider.name(),
dataProvider);
+ event.addAttribute(AttributeName.STATEFUL_REBALANCER.name(),
readOnlyWagedRebalancer);
+
+ try {
+ // Run the required stages to obtain the BestPossibleOutput
+ RebalanceUtil.runStage(event, new ResourceComputationStage());
+ RebalanceUtil.runStage(event, new CurrentStateComputationStage());
+ RebalanceUtil.runStage(event, new BestPossibleStateCalcStage());
+ } catch (Exception e) {
+ LOG.error("getIdealAssignmentForWagedFullAuto(): Failed to compute
ResourceAssignments!", e);
+ } finally {
+ // Close all ZK connections
+ baseDataAccessor.close();
Review comment:
It shouldn't fail, but I can wrap the entire logic in a try-catch as
well.
##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -140,6 +160,81 @@ public static String serializeByComma(List<String>
objects) {
}
}
+ /**
+ * Returns the expected ideal ResourceAssignments for the given resources in
the cluster
+ * calculated using the read-only WAGED rebalancer.
+ * @param metadataStoreAddress
+ * @param clusterConfig
+ * @param instanceConfigs
+ * @param liveInstances
+ * @param newIdealStates
+ * @param newResourceConfigs
+ * @return
+ */
+ public static Map<String, ResourceAssignment>
getIdealAssignmentForWagedFullAuto(
+ String metadataStoreAddress, ClusterConfig clusterConfig,
+ List<InstanceConfig> instanceConfigs, List<String> liveInstances,
+ List<IdealState> newIdealStates, List<ResourceConfig>
newResourceConfigs) {
+ // Prepare a data accessor for a dataProvider (cache) refresh
+ BaseDataAccessor<ZNRecord> baseDataAccessor = new
ZkBaseDataAccessor<>(metadataStoreAddress);
+ HelixDataAccessor helixDataAccessor =
+ new ZKHelixDataAccessor(clusterConfig.getClusterName(),
baseDataAccessor);
+
+ // Obtain a refreshed dataProvider (cache) and overwrite cluster
parameters with the given parameters
+ ResourceControllerDataProvider dataProvider =
+ new ResourceControllerDataProvider(clusterConfig.getClusterName());
+ dataProvider.requireFullRefresh();
+ dataProvider.refresh(helixDataAccessor);
+ dataProvider.setClusterConfig(clusterConfig);
+ dataProvider.setInstanceConfigMap(instanceConfigs.stream()
Review comment:
@jiajunwang
I've considered the case you mentioned, and what we have here is I believe
the right thing to do. We should overwrite completely (replace), that way, we
could allow users to keep, modify, or remove certain items in the Collection.
Users still have the ability to retrieve the existing ResourceConfigs in the
cluster, so that's not a big concern.
Moreover, this fits nicely with the API methods already provided in the
DataProvider interface. If we want to support merge or remove, that would
require further change in the DataProvider interface, which is undesirable
because we'd be making API changes for the sake of having a util method or for
testing.
##########
File path: helix-core/src/main/java/org/apache/helix/util/RebalanceUtil.java
##########
@@ -164,4 +167,25 @@ public static void scheduleOnDemandPipeline(String
clusterName, long delay,
clusterName);
}
}
+
+ /**
+ * runStage allows the run of individual stages. It can be used to mock a
part of the Controller
+ * pipeline run.
+ *
+ * An example usage is as follows:
+ * runStage(event, new ResourceComputationStage());
+ * runStage(event, new CurrentStateComputationStage());
+ * runStage(event, new BestPossibleStateCalcStage());
+ * By running these stages, we are able to obtain BestPossibleStateOutput in
the event object.
+ * @param event
+ * @param stage
+ * @throws Exception
+ */
+ public static void runStage(ClusterEvent event, Stage stage) throws
Exception {
Review comment:
I have considered this option, and I decided against it because we lose
generality. The nature of runStage sometimes requires the modification of which
stages to run and depending on the test, it requires the tests to
add/modify/remove Attributes in ClusterEvent. So this is the right granularity.
If we provide some composite method like runStages that runs multiple
stages, the use case for it would be limited and not as configurable.
##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -140,6 +160,81 @@ public static String serializeByComma(List<String>
objects) {
}
}
+ /**
+ * Returns the expected ideal ResourceAssignments for the given resources in
the cluster
+ * calculated using the read-only WAGED rebalancer.
+ * @param metadataStoreAddress
+ * @param clusterConfig
+ * @param instanceConfigs
+ * @param liveInstances
+ * @param newIdealStates
+ * @param newResourceConfigs
+ * @return
+ */
+ public static Map<String, ResourceAssignment>
getIdealAssignmentForWagedFullAuto(
Review comment:
Already reviewed that option, but if we were to place this in
RebalanceUtil, we should move the existing function there too, but that would
cause a backward-incompatible change. Let's keep it in here for consistency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
