You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2017/12/28 14:57:55 UTC
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2485
STORM-2859: Fix a number of issues with NormalizedResources when reso…
…urce totals are zero
Issues fixed:
* calculateMinPercentageUsedBy threw division by 0 error if total cpu or memory was 0.
* remove was quietly skipping any resources that was present in only one resource list. I think we should make sure to cover all resources in the input list, so add/remove behave like normal vector addition.
* Added check that all resources in a rack/node are also present in the total when doing calculateAvg and calculateMin.
* calculateAvg was being inconsistent about the divisor in the average between cpu/memory and other resources. Fixed so resources that are not accounted for in the total are also not counted in the divisor.
* calculateMin didn't handle resource totals being 0 very well. The method defined `a/0 = 0`, so if any resource in the total had a 0 value, the method would return 0 and all racks/nodes would end up being prioritized equally. Changed calculateMin and calculateAvg to skip any resources where the total of that resource is 0. I think it makes sense to disregard such resources and fall back on prioritizing by any remaining non-0 resources. calculateAvg actually already did this for cpu and memory. In case there are no non-0 resources in the total, we can just return 100% for both average and minimum to disable prioritization, since any rack/node is as good as any other.
* Fixed up NormalizedResources constructor so it doesn't call an overridable method.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-2859
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2485.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2485
----
commit 8f2a66d23e382d22fdc6a4ee32a53b442f2e153a
Author: Stig Rohde Døssing <sr...@...>
Date: 2017-12-13T15:47:31Z
STORM-2859: Fix a number of issues with NormalizedResources when resource totals are zero
----
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159973149
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
--- End diff --
This is fine, just make sure the java docs explain how this should be used and when it will throw an exception.
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159735761
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
--- End diff --
My assumption was that if a resource is not around then it's value is 0. It looks like in this case, that if a resources is missing an exception will be thrown. I am not sure that this will not happen in normal operation, or if a user submits a topology with a resource that is not currently supported.
---
[GitHub] storm issue #2485: STORM-2859: Fix a number of issues with NormalizedResourc...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2485
@revans2 I took another look after some of your last comments, and found some more bugs. I spent some time splitting up NormalizedResources so it was easier to test. Your comments should also have been addressed.
Most of the classes related to NormalizedResources have been moved to a new package. The move is in the last commit, so it's probably easier to exclude that one from the diff.
If you have an idea for a better place to put the static fields in NormalizedResources, I'd be happy to move them. They are referred to from a lot of different classes, and it would be better if we could create the classes somewhere during startup and inject them into the classes that need them instead of referring to these static fields.
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159736224
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
+ }
+
+ for (int i = 0; i < otherResources.length; i++) {
+ double totalValue = otherResources[i];
+ if (totalValue == 0.0) {
+ //Skip any resources where the total is 0, the percent used for this resource isn't meaningful.
+ //We fall back to prioritizing by cpu, memory and any other resources by ignoring this value
+ skippedResourceTypes++;
+ continue;
}
+ double usedValue;
+ if (i >= used.otherResources.length) {
+ //Resources missing from used are using none of that resource
+ usedValue = 0.0;
+ } else {
+ usedValue = used.otherResources[i];
+ }
+ total += usedValue / totalValue;
+ }
+ //Adjust the divisor for the average to account for any skipped resources (those where the total was 0)
+ int divisor = 2 + otherResources.length - skippedResourceTypes;
+ if (divisor == 0) {
+ /*This is an arbitrary choice to make the result consistent with calculateMin.
+ Any value would be valid here, becase there are no (non-zero) resources in the total set of resources,
+ so we're trying to average 0 values.
+ */
+ return 100.0;
+ } else {
+ return (total * 100.0) / divisor;
}
- //To get the count we divide by we need to take the maximum length because we are doing an average.
- return (total * 100.0) / (2 + Math.max(otherResources.length, used.otherResources.length));
}
/**
- * Calculate the minimum resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the minimum resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the minimum percentage used 0.0 to 100.0.
+ * @return the minimum percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total.
*/
public double calculateMinPercentageUsedBy(NormalizedResources used) {
double totalMemory = getTotalMemoryMb();
double totalCpu = getTotalCpu();
+
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating min percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
+ }
+
if (used.otherResources.length != otherResources.length
|| totalMemory == 0.0
|| totalCpu == 0.0) {
//If the lengths don't match one of the resources will be 0, which means we would calculate the percentage to be 0.0
// and so the min would be 0.0 (assuming that we can never go negative on a resource being used.
return 0.0;
}
- double min = used.getTotalMemoryMb() / totalMemory;
- min = Math.min(min, used.getTotalCpu() / getTotalCpu());
+
+ double min = 100.0;
+ if (totalMemory != 0.0) {
+ min = Math.min(min, used.getTotalMemoryMb() / totalMemory);
+ }
+ if (totalCpu != 0.0) {
--- End diff --
Again the math looks OK, but could you explain that 0 % used is ignored in the javadocs, because it is not obvious to me that it what will happen.
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159972982
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -184,30 +191,25 @@ public void remove(NormalizedResources other) {
int otherLength = other.otherResources.length;
int length = otherResources.length;
if (otherLength > length) {
- double [] newResources = new double[otherLength];
+ double[] newResources = new double[otherLength];
System.arraycopy(newResources, 0, otherResources, 0, length);
otherResources = newResources;
}
- for (int i = 0; i < Math.min(length, otherLength); i++) {
+ for (int i = 0; i < otherLength; i++) {
--- End diff --
With where remove is used it should never go negative, so I would be fine with blowing up for all negative values so long as we call it out in the javadocs, and treat something that is not there as a 0. I agree there is a lot about the code that is not ideal.
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159735958
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
+ }
+
+ for (int i = 0; i < otherResources.length; i++) {
+ double totalValue = otherResources[i];
+ if (totalValue == 0.0) {
+ //Skip any resources where the total is 0, the percent used for this resource isn't meaningful.
+ //We fall back to prioritizing by cpu, memory and any other resources by ignoring this value
+ skippedResourceTypes++;
--- End diff --
This looks fine to me, but could we explain in the javadoc for this method that 0 values are handled differently then might be expected?
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2485
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159957666
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
--- End diff --
I could only find references to this method and calculateMin in the two resource aware strategies, where they seem to be used to estimate how large a percentage of total cluster resources a rack or node represents. That's why I assumed that hitting this case would always be a bug, because the total cluster resources should contain all the resources of the racks.
I'm happy to instead pretend that otherResources has been padded with zeroes, and skip the zero values like we do a few lines down.
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159734646
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -184,30 +191,25 @@ public void remove(NormalizedResources other) {
int otherLength = other.otherResources.length;
int length = otherResources.length;
if (otherLength > length) {
- double [] newResources = new double[otherLength];
+ double[] newResources = new double[otherLength];
System.arraycopy(newResources, 0, otherResources, 0, length);
otherResources = newResources;
}
- for (int i = 0; i < Math.min(length, otherLength); i++) {
+ for (int i = 0; i < otherLength; i++) {
--- End diff --
I think that other may have a longer array, and if that happens then we will get an ArrayIndexOutOfBoundsException.
```
NormalizedResources A = new NormalizedResources({"cpu": 1});
NormalizedResources B = new NormalizedResources({"cpu":1, "foo": 100});
A.remove(B);
```
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159952878
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -184,30 +191,25 @@ public void remove(NormalizedResources other) {
int otherLength = other.otherResources.length;
int length = otherResources.length;
if (otherLength > length) {
- double [] newResources = new double[otherLength];
+ double[] newResources = new double[otherLength];
System.arraycopy(newResources, 0, otherResources, 0, length);
otherResources = newResources;
}
- for (int i = 0; i < Math.min(length, otherLength); i++) {
+ for (int i = 0; i < otherLength; i++) {
--- End diff --
Yes. I'm not sure how we should handle removes if `other` contains values not in `this`. The function was previously just ignoring the "foo" value in B, which I don't think makes that much sense.
It makes add/remove act unlike regular set addition/subtraction, `A.remove(B).add(B) != A` for example. Behavior is also different between having 0 of a resource, and not having the resource, e.g. if `A = {"cpu": 1, "foo": 0}`, we would instead get an assertion error when removing B.
If we want to actually subtract "foo" from A, we need to allow negative values in the result. Maybe we could delay the no-negative-values check to some other point, so we just disallow negative values in a final result but not in intermediate calculations?
What do you think we should do?
---
[GitHub] storm pull request #2485: STORM-2859: Fix a number of issues with Normalized...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2485#discussion_r159957760
--- Diff: storm-server/src/main/java/org/apache/storm/scheduler/resource/NormalizedResources.java ---
@@ -246,59 +259,133 @@ public boolean couldHoldIgnoringSharedMemory(NormalizedResources other) {
return true;
}
+ private void throwBecauseResourceIsMissingFromTotal(int resourceIndex) {
+ String resourceName = null;
+ for (Map.Entry<String, Integer> entry : resourceNames.entrySet()) {
+ int index = entry.getValue();
+ if (index == resourceIndex) {
+ resourceName = entry.getKey();
+ break;
+ }
+ }
+ if (resourceName == null) {
+ throw new IllegalStateException("Array index " + resourceIndex + " is not mapped in the resource names map."
+ + " This should not be possible, and is likely a bug in the Storm code.");
+ }
+ throw new IllegalArgumentException("Total resources does not contain resource '"
+ + resourceName
+ + "'. All resources should be represented in the total. This is likely a bug in the Storm code");
+ }
+
/**
- * Calculate the average resource usage percentage with this being the total resources and
- * used being the amounts used.
+ * Calculate the average resource usage percentage with this being the total resources and used being the amounts used.
+ *
* @param used the amount of resources used.
- * @return the average percentage used 0.0 to 100.0.
+ * @return the average percentage used 0.0 to 100.0. Clamps to 100.0 in case there are no available resources in the total
*/
public double calculateAveragePercentageUsedBy(NormalizedResources used) {
+ int skippedResourceTypes = 0;
double total = 0.0;
double totalMemory = getTotalMemoryMb();
if (totalMemory != 0.0) {
total += used.getTotalMemoryMb() / totalMemory;
+ } else {
+ skippedResourceTypes++;
}
double totalCpu = getTotalCpu();
if (totalCpu != 0.0) {
total += used.getTotalCpu() / getTotalCpu();
+ } else {
+ skippedResourceTypes++;
}
- //If total is 0 we add in a 0% used, so we can just skip over anything that is not in both.
- int length = Math.min(used.otherResources.length, otherResources.length);
- for (int i = 0; i < length; i++) {
- if (otherResources[i] != 0.0) {
- total += used.otherResources[i] / otherResources[i];
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Calculating avg percentage used by. Used CPU: {} Total CPU: {} Used Mem: {} Total Mem: {}"
+ + " Other Used: {} Other Total: {}", totalCpu, used.getTotalCpu(), totalMemory, used.getTotalMemoryMb(),
+ this.toNormalizedOtherResources(), used.toNormalizedOtherResources());
+ }
+
+ if (used.otherResources.length > otherResources.length) {
+ throwBecauseResourceIsMissingFromTotal(used.otherResources.length);
+ }
+
+ for (int i = 0; i < otherResources.length; i++) {
+ double totalValue = otherResources[i];
+ if (totalValue == 0.0) {
+ //Skip any resources where the total is 0, the percent used for this resource isn't meaningful.
+ //We fall back to prioritizing by cpu, memory and any other resources by ignoring this value
+ skippedResourceTypes++;
--- End diff --
Yes, I'll add a note to both methods.
---