Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-172883855
OK so Math.abs(Integer.MIN_VALUE) actually returns Integer.MIN_VALUE
because of an integer overflow error. That is a good corner case to remember
in the future.
@arunmahadevan `Math.abs(val.hasCode() % numPartitions)` would work too.
The only reason to do one over the other would be performance vs readability.
In my very unscientific test
```
public class Test {
public static int toPositive(int number) {
return number & Integer.MAX_VALUE;
}
public static void main(String [] args) {
int count = Integer.valueOf(args[0]);
long start = System.nanoTime();
for (int i = 0; i < count; i++) {
int ret = Math.abs(i);
//int ret = toPositive(i);
}
long end = System.nanoTime();
System.out.println(end+" - "+ start+" = "+(end-start)+" rate:
"+(((double)count)/(end-start)));
}
}
```
I get about 291 ops per nanosecond for toPositive
`1453216134803090000 - 1453216134799657000 = 3433000 rate:
291.29041654529567`
and 289 ops per nanosecond for Math.abs
`1453216159561620000 - 1453216159558157000 = 3463000 rate:
288.7669650591972`
To me it is such a small difference About 1% over a part of the code where
computing the hash code dominates that it should come down to readability.
Both look equally as readable to me (because of the comments on toPositive).
I am +1 for this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
