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?
---
