[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
GitHub user d2r opened a pull request: https://github.com/apache/storm/pull/881 [STORM-1208] Guard against NPE, and avoid using NaN values * Removes malformed preconditions that don't do anything, and if they did work would throw Errors that would crash nimbus. * When reading and writing average-type metrics, use `0` instead of `NaN` * Avoid NPE accessing some maps by doing `(get m k)` instead of `(m k)`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/d2r/storm storm-1208-ui-npe-nan Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/881.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 #881 commit 3857644b4c459cbe42b78ca8a13bd55ef704fd30 Author: Derek Dagit Date: 2015-11-16T20:15:15Z Guard against NPE, and avoid using NaN values --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user kishorvpatil commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157164426 LGTM. +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user redsanket commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157175212 +1 NB --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user zhuoliu commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157178283 Nice fix. +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user hustfxj commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157268072 it looks good --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user wuchong commented on a diff in the pull request: https://github.com/apache/storm/pull/881#discussion_r45027342 --- Diff: storm-core/src/jvm/backtype/storm/metric/internal/LatencyStatAndMetric.java --- @@ -242,7 +248,7 @@ private synchronized void rotate(long lat, long count, long timeSpent, long targ totalCount += countBuckets[i]; timeNeeded -= bucketTime[i]; } -return ((double)totalLat)/totalCount; +return totalCount > 0 ? ((double)totalLat)/totalCount : 0.0; --- End diff -- Maybe we can pull the code out into a function named like `div-or-0`, as it appears more than once. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user d2r commented on a diff in the pull request: https://github.com/apache/storm/pull/881#discussion_r45077073 --- Diff: storm-core/src/jvm/backtype/storm/metric/internal/LatencyStatAndMetric.java --- @@ -242,7 +248,7 @@ private synchronized void rotate(long lat, long count, long timeSpent, long targ totalCount += countBuckets[i]; timeNeeded -= bucketTime[i]; } -return ((double)totalLat)/totalCount; +return totalCount > 0 ? ((double)totalLat)/totalCount : 0.0; --- End diff -- Yes we should do this. In fact, while reading the documentation again, it seems we must guard against both values of `NaN` and `Infinity`. ``` user=> (/ 0. 0.) NaN user=> (/ 1. 0.) Infinity ``` We have only seen the NaN case recently, but it would not hurt to handle both cases, since the dividend and divisor could be 0 independently, especially since the metrics are sampled independently. I will update the code to refactor and to check both. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user d2r commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157440029 @hustfxj , @kishorvpatil , @zhuoliu , @redsanket, Added checking for `Infinity` plus some refactoring as per @hustfxj's review. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user kishorvpatil commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157540452 still +1. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user wuchong commented on the pull request: https://github.com/apache/storm/pull/881#issuecomment-157572950 +1 LGTM --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/881#discussion_r45250004
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -335,30 +358,21 @@
(defn- agg-bolt-streams-lat-and-count
"Aggregates number executed and process & execute latencies."
[idk->exec-avg idk->proc-avg idk->executed]
- {:pre (apply = (map #(set (keys %))
- [idk->exec-avg
- idk->proc-avg
- idk->executed]))}
- (letfn [(weight-avg [id avg] (let [num-e (idk->executed id)]
- (if (and avg num-e)
- (* avg num-e)
- 0)))]
+ (letfn [(weight-avg [id avg]
+(let [num-e (idk->executed id)]
+ (product-or-0 avg num-e)))]
(into {}
(for [k (keys idk->exec-avg)]
-[k {:executeLatencyTotal (weight-avg k (idk->exec-avg k))
-:processLatencyTotal (weight-avg k (idk->proc-avg k))
+[k {:executeLatencyTotal (weight-avg k (get idk->exec-avg k))
+:processLatencyTotal (weight-avg k (get idk->proc-avg k))
--- End diff --
what is the point of adding the get especially for (get idk->exec-avg k)?
you are getting the keys from the map and then accessing it right? the keys
cannot possibly not exist in the map
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/881 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-1208] Guard against NPE, and avoid usin...
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/881#discussion_r45251619
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -335,30 +358,21 @@
(defn- agg-bolt-streams-lat-and-count
"Aggregates number executed and process & execute latencies."
[idk->exec-avg idk->proc-avg idk->executed]
- {:pre (apply = (map #(set (keys %))
- [idk->exec-avg
- idk->proc-avg
- idk->executed]))}
- (letfn [(weight-avg [id avg] (let [num-e (idk->executed id)]
- (if (and avg num-e)
- (* avg num-e)
- 0)))]
+ (letfn [(weight-avg [id avg]
+(let [num-e (idk->executed id)]
+ (product-or-0 avg num-e)))]
(into {}
(for [k (keys idk->exec-avg)]
-[k {:executeLatencyTotal (weight-avg k (idk->exec-avg k))
-:processLatencyTotal (weight-avg k (idk->proc-avg k))
+[k {:executeLatencyTotal (weight-avg k (get idk->exec-avg k))
+:processLatencyTotal (weight-avg k (get idk->proc-avg k))
--- End diff --
```
user=> (def nilmap nil)
#'user/nilmap
user=> (get nilmap :somekey)
nil
user=> (nilmap :somekey)
NullPointerException user/eval3 (NO_SOURCE_FILE:4)
```
`get` guards against the map itself is `nil`.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
