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 2020/07/22 17:24:21 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1167: Change getBaselineAasignment to account for partial-WAGED clusters
jiajunwang commented on a change in pull request #1167:
URL: https://github.com/apache/helix/pull/1167#discussion_r458956773
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -643,23 +643,28 @@ private void validateInput(ResourceControllerDataProvider clusterData,
private Map<String, ResourceAssignment> getBaselineAssignment(
AssignmentMetadataStore assignmentMetadataStore, CurrentStateOutput currentStateOutput,
Set<String> resources) throws HelixRebalanceException {
- Map<String, ResourceAssignment> currentBaseline = Collections.emptyMap();
+ Map<String, ResourceAssignment> currentBaseline = new HashMap<>();
if (assignmentMetadataStore != null) {
try {
_stateReadLatency.startMeasuringLatency();
- currentBaseline = assignmentMetadataStore.getBaseline();
+ currentBaseline = new HashMap<>(assignmentMetadataStore.getBaseline());
_stateReadLatency.endMeasuringLatency();
} catch (Exception ex) {
throw new HelixRebalanceException(
"Failed to get the current baseline assignment because of unexpected error.",
HelixRebalanceException.Type.INVALID_REBALANCER_STATUS, ex);
}
}
- if (currentBaseline.isEmpty()) {
+ currentBaseline.keySet().retainAll(resources);
+ if (!currentBaseline.keySet().containsAll(resources)) {
Review comment:
containsAll() will iterate through resources anyway. So just do the loop without check here.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -643,23 +643,28 @@ private void validateInput(ResourceControllerDataProvider clusterData,
private Map<String, ResourceAssignment> getBaselineAssignment(
AssignmentMetadataStore assignmentMetadataStore, CurrentStateOutput currentStateOutput,
Set<String> resources) throws HelixRebalanceException {
- Map<String, ResourceAssignment> currentBaseline = Collections.emptyMap();
+ Map<String, ResourceAssignment> currentBaseline = new HashMap<>();
if (assignmentMetadataStore != null) {
try {
_stateReadLatency.startMeasuringLatency();
- currentBaseline = assignmentMetadataStore.getBaseline();
+ currentBaseline = new HashMap<>(assignmentMetadataStore.getBaseline());
_stateReadLatency.endMeasuringLatency();
} catch (Exception ex) {
throw new HelixRebalanceException(
"Failed to get the current baseline assignment because of unexpected error.",
HelixRebalanceException.Type.INVALID_REBALANCER_STATUS, ex);
}
}
- if (currentBaseline.isEmpty()) {
+ currentBaseline.keySet().retainAll(resources);
+ if (!currentBaseline.keySet().containsAll(resources)) {
LOG.warn("The current baseline assignment record is empty. Use the current states instead.");
- currentBaseline = currentStateOutput.getAssignment(resources);
+ for (Map.Entry<String, ResourceAssignment> entry : currentStateOutput.getAssignment(resources)
+ .entrySet()) {
+ if (resources.contains(entry.getKey()) && !currentBaseline.containsKey(entry.getKey())) {
Review comment:
currentBaseline.putIfAbsent() ?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -643,23 +643,28 @@ private void validateInput(ResourceControllerDataProvider clusterData,
private Map<String, ResourceAssignment> getBaselineAssignment(
AssignmentMetadataStore assignmentMetadataStore, CurrentStateOutput currentStateOutput,
Set<String> resources) throws HelixRebalanceException {
- Map<String, ResourceAssignment> currentBaseline = Collections.emptyMap();
+ Map<String, ResourceAssignment> currentBaseline = new HashMap<>();
if (assignmentMetadataStore != null) {
try {
_stateReadLatency.startMeasuringLatency();
- currentBaseline = assignmentMetadataStore.getBaseline();
+ currentBaseline = new HashMap<>(assignmentMetadataStore.getBaseline());
_stateReadLatency.endMeasuringLatency();
} catch (Exception ex) {
throw new HelixRebalanceException(
"Failed to get the current baseline assignment because of unexpected error.",
HelixRebalanceException.Type.INVALID_REBALANCER_STATUS, ex);
}
}
- if (currentBaseline.isEmpty()) {
+ currentBaseline.keySet().retainAll(resources);
+ if (!currentBaseline.keySet().containsAll(resources)) {
LOG.warn("The current baseline assignment record is empty. Use the current states instead.");
- currentBaseline = currentStateOutput.getAssignment(resources);
+ for (Map.Entry<String, ResourceAssignment> entry : currentStateOutput.getAssignment(resources)
Review comment:
getAssignment() is very expensive, let's do it for the missing resources only. If you do it for all resources, the algorithm performance would be impacted.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -643,23 +643,28 @@ private void validateInput(ResourceControllerDataProvider clusterData,
private Map<String, ResourceAssignment> getBaselineAssignment(
AssignmentMetadataStore assignmentMetadataStore, CurrentStateOutput currentStateOutput,
Set<String> resources) throws HelixRebalanceException {
- Map<String, ResourceAssignment> currentBaseline = Collections.emptyMap();
+ Map<String, ResourceAssignment> currentBaseline = new HashMap<>();
if (assignmentMetadataStore != null) {
try {
_stateReadLatency.startMeasuringLatency();
- currentBaseline = assignmentMetadataStore.getBaseline();
+ currentBaseline = new HashMap<>(assignmentMetadataStore.getBaseline());
_stateReadLatency.endMeasuringLatency();
} catch (Exception ex) {
throw new HelixRebalanceException(
"Failed to get the current baseline assignment because of unexpected error.",
HelixRebalanceException.Type.INVALID_REBALANCER_STATUS, ex);
}
}
- if (currentBaseline.isEmpty()) {
+ currentBaseline.keySet().retainAll(resources);
+ if (!currentBaseline.keySet().containsAll(resources)) {
LOG.warn("The current baseline assignment record is empty. Use the current states instead.");
Review comment:
The current baseline assignment record does not contain all resources assignment....
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org