You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by vesense <gi...@git.apache.org> on 2016/01/16 13:02:11 UTC
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
GitHub user vesense opened a pull request:
https://github.com/apache/storm/pull/1023
[STORM-1481] avoid Math.abs(Integer) get a negative value
https://issues.apache.org/jira/browse/STORM-1481
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vesense/storm STORM-1481
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1023.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 #1023
----
commit 181b3b09a973acf99719ad60fe03f4d23c869324
Author: Xin Wang <be...@163.com>
Date: 2016-01-16T11:40:24Z
avoid abs get negative value
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-173345063
@revans2 I'll take care of the merge to 0.10.x as soon as this is in master.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1023
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-173343533
Because the files have change locations porting to 0.10.x is going to need a separate pull request (git is not happy)
I will merge this in for master and 1.x-branch
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-172423672
+1.
The fix is necessary due to follows, which can cause problem during grouping.
Integer.MAX_VALUE % 1 = 0
Integer.MAX_VALUE % 2 = 0
Integer.MAX_VALUE % 3 = -2
Integer.MAX_VALUE % 4 = 0
Integer.MAX_VALUE % 5 = -3
Integer.MAX_VALUE % 6 = -2
Integer.MAX_VALUE % 7 = -2
Integer.MAX_VALUE % 8 = 0
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-172426904
why not `Math.abs(val.hashCode() % numPartitions)` ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-173287607
@vesense I don't see any indication in the JIRA which branches you want this fix merged into. master and 1.x seem simple enough, but it appears to be a bug in 10.x and 9.x releases too. I'm not sure it is critical enough to go back to either of them, but I wanted to check with you before merging this in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-173345862
@ptgoetz sounds good just running a sanity check with the tests before merging it in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1481] avoid Math.abs(Integer) get a neg...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1023#issuecomment-173310578
+1
I wouldn't mind seeing this go back to 1.x and 0.10.x (0.10.x has a few other minor fixes). But I don't think we need to support 0.9.x too much longer.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---