You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by wangli1426 <gi...@git.apache.org> on 2015/09/22 15:06:31 UTC
[GitHub] storm pull request: [STORM-1057] Add throughput metrics to spouts/...
GitHub user wangli1426 opened a pull request:
https://github.com/apache/storm/pull/753
[STORM-1057] Add throughput metrics to spouts/bolts and display them on web ui
The throughputs for the spouts and bolts could help the user to identify the performance bottleneck and detect the load balancing issue. In this RP, I take measurements on the throughput of the executors and display them on web UI.
### Summary of Changes
1. Take throughput measurements on the spouts and bolts;
2. Add throughput to ExecutorStats;
3. Display the throughputs on web UI.
**Note: If you cannot see the throughputs on your web UI, please clean your browser cache and try again.**
### Screenshots
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wangli1426/storm throughput-metrics
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/753.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 #753
----
commit 67721f77f28cbb4a29fab7e3023b220095886338
Author: Li Wang <wa...@gmail.com>
Date: 2015-09-18T02:11:20Z
initializing Timer in RateTracker.java lazily to avoid a bug in Clojure Maven plugin that might make compiling process never finish
commit 1dad2041fb0ae8689794fec93d05df2266292dc8
Author: Li Wang <wa...@gmail.com>
Date: 2015-09-18T07:19:28Z
take throughput measurements on spouts and bolts
commit d74f784175cc4b8c631c52acaab3e0743f11a56d
Author: Li Wang <wa...@gmail.com>
Date: 2015-09-20T01:26:11Z
keep Nimbus updated to the latest throughput stats
commit 1a2a9b38cd256b29cb0177450f29d03ce20471c3
Author: Li Wang <wa...@gmail.com>
Date: 2015-09-21T05:27:05Z
display throughput metrics on web UI
----
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148289930
@revans2
Thank you for your comment. I will up-merge this PR. However, I can't quite understand your last sentence. What do you mean by "you like to see the thrift code changed"? Could please explain more about it? Thanks.
---
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 issue #753: [STORM-1057] Add throughput metrics to spouts/bolts and di...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the issue:
https://github.com/apache/storm/pull/753
@harshach I managed to upmerge this PR to 1.x-branch in #1703. Please review. Thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40848077
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
* @param count number of arrivals
*/
public void notify(long count) {
- _histograms[_histograms.length-1]+=count;
+ _histograms[_numOfSlides - 1] += count;
--- End diff --
Never mind I filed STORM-1078 myself.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-145194180
@revans2,
Thanks for your detailed comments. I understand your concern that we might not need to use a new system to obtain the throughput metric, given that we already have code in stats.clj doing the same thing. I will try to obtain the throughput metric using existing functions in stats.clj and come back when I finish.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40846010
--- Diff: storm-core/src/storm.thrift ---
@@ -204,6 +203,7 @@ struct ExecutorStats {
2: required map<string, map<string, i64>> transferred;
3: required ExecutorSpecificStats specific;
4: required double rate;
+ 5: required map<string, map<string, double>> throughput;
--- End diff --
If we want to be able to add this as a rolling upgrade this needs to be optional, and we need the code everywhere to be able to handle when it is not provided.
To me this feature is not important enough to force downtime in a cluster.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144539829
@wangli1426 I am done with my first pass through the code. There may be other things in here too that I missed on the first pass.
---
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-1057] Add throughput metrics to spouts/...
Posted by unsleepy22 <gi...@git.apache.org>.
Github user unsleepy22 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r46569121
--- Diff: storm-core/src/clj/backtype/storm/daemon/executor.clj ---
@@ -339,7 +341,7 @@
(if (seq data-points)
(task/send-unanchored task-data Constants/METRICS_STREAM_ID [task-info data-points] overflow-buffer))))
([executor-data task-data ^TupleImpl tuple]
- (metrics-tick executor-data task-data tuple nil)
+ (metrics-tick executor-data task-data tuple nil)
--- End diff --
indent by 1 space
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144543081
Actually I thought about this a bit more and I am not sure what value this adds over dividing the emitted counts that we already have by the time window that they are for. I don't see a reason to add in more code to get a number we already know. I am -1 for this approach. I am fine with doing the calculation for the rate and displaying it in the UI. That would be helpful, but I don't see a reason to try and collect it again, in a different way.
---
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 #753: [STORM-1057] Add throughput metrics to spouts/bolts...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/753
---
[GitHub] storm pull request: [STORM-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42243045
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -626,7 +638,8 @@
(let [^CommonAggregateStats cas (.get_common_stats stats)]
{"stream" stream-id
"emitted" (nil-to-zero (.get_emitted cas))
- "transferred" (nil-to-zero (.get_transferred cas))}))
+ "transferred" (nil-to-zero (.get_transferred cas))
+ "throughput" (float-str (.get_throughput cas))}))
--- End diff --
And in several other places below too.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-149948793
Hi @revans2 ,
Recent commits to the master branch cause conflict to my PR, so I up-merged my PR in 4de33d1. Could you please review the code again? Thank you very much.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148178136
@wangli1426 sorry it took so long to respond. The code looks a lot simpler. Please upmerge. Stats aggregation has changed places, but it still looks like it is a not too difficult change.
I also would really like to see the thrift code changed so adding in the throughput can be a rolling upgrade.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42671953
--- Diff: STORM-UI-REST-API.md ---
@@ -351,11 +354,13 @@ Sample response:
"executors": 12,
"emitted": 184580,
"transferred": 0,
+ "throughput": "195.000",
"acked": 184640,
"executeLatency": "0.048",
"tasks": 12,
"executed": 184620,
"processLatency": "0.043",
+ "throughput":
--- End diff --
This seems to be missing a 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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42243012
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -626,7 +638,8 @@
(let [^CommonAggregateStats cas (.get_common_stats stats)]
{"stream" stream-id
"emitted" (nil-to-zero (.get_emitted cas))
- "transferred" (nil-to-zero (.get_transferred cas))}))
+ "transferred" (nil-to-zero (.get_transferred cas))
+ "throughput" (float-str (.get_throughput cas))}))
--- End diff --
I think we need nil-to-zero here as well, unless float-str will do that already
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42243189
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -205,6 +205,9 @@
[stats stream amt]
(update-executor-stat! stats [:common :transferred] stream (* (stats-rate stats) amt)))
+(defn update-stats-throughput! [stats stream throughput]
+ (update-executor-stat! stats [:common :throughput] stream (* (stats-rate stats) throughput)))
+
--- End diff --
This entire function is not needed, and is not used. It should be removed.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40910516
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
* @param count number of arrivals
*/
public void notify(long count) {
- _histograms[_histograms.length-1]+=count;
+ _histograms[_numOfSlides - 1] += count;
--- End diff --
I was planning on doing it today, because I had already pulled it into my distribution, and it should not be that hard to do. It is mostly switching the _histograms to be an array of AtomicLong instead of regular long, and then making sure that the current bucket is modified using getAndSet so that there is no read modify write that is not atomic. I was also thinking I would do something so that the current bucket does not require any offset calculations, but that is a very small optimization, that once the JIT kicks in probably would not make any difference.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40907742
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
* @param count number of arrivals
*/
public void notify(long count) {
- _histograms[_histograms.length-1]+=count;
+ _histograms[_numOfSlides - 1] += count;
--- End diff --
@revans2
Thanks for your comment. Will you solve [STORM-1078] by yourself? I can solve this bug if you are too busy to do it.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42242430
--- Diff: storm-core/src/storm.thrift ---
@@ -244,8 +244,9 @@ struct CommonAggregateStats {
2: optional i32 num_tasks;
3: optional i64 emitted;
4: optional i64 transferred;
-5: optional i64 acked;
-6: optional i64 failed;
+5: optional double throughput;
+6: optional i64 acked;
+7: optional i64 failed;
--- End diff --
This is another problem with maintaining binary compatibility with the previous code. You cannot renumber entries. The tags at the beginning are what identify the field in the binary data. By renumbering them the new code and old code will mix up throughput, acked, and failed.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42259569
--- Diff: storm-core/src/storm.thrift ---
@@ -244,8 +244,9 @@ struct CommonAggregateStats {
2: optional i32 num_tasks;
3: optional i64 emitted;
4: optional i64 transferred;
-5: optional i64 acked;
-6: optional i64 failed;
+5: optional double throughput;
+6: optional i64 acked;
+7: optional i64 failed;
--- End diff --
Thanks for the comment and point taken.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40850076
--- Diff: storm-core/src/clj/backtype/storm/daemon/executor.clj ---
@@ -570,7 +575,11 @@
(tasks-fn out-stream-id values))
rooted? (and message-id has-ackers?)
root-id (if rooted? (MessageId/generateId rand))
- out-ids (fast-list-for [t out-tasks] (if rooted? (MessageId/generateId rand)))]
+ out-ids (fast-list-for [t out-tasks] (if rooted? (MessageId/generateId rand)))
+ sampler? (sampler)]
+ (when sampler?
+ (.notify rate-tracker (count out-tasks))
--- End diff --
Wait I just thought about this again. How in the world can we get an accurate rate if we are only counting a subset of the tuples? Wouldn't we also have to increase the rate proportionally to the sampling rate?
---
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-1057] Add throughput metrics to spouts/...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197118972
@revans2 @d2r @redsanket can you comment. thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40849246
--- Diff: storm-core/src/clj/backtype/storm/daemon/executor.clj ---
@@ -570,7 +575,11 @@
(tasks-fn out-stream-id values))
rooted? (and message-id has-ackers?)
root-id (if rooted? (MessageId/generateId rand))
- out-ids (fast-list-for [t out-tasks] (if rooted? (MessageId/generateId rand)))]
+ out-ids (fast-list-for [t out-tasks] (if rooted? (MessageId/generateId rand)))
+ sampler? (sampler)]
+ (when sampler?
+ (.notify rate-tracker (count out-tasks))
+ (stats/update-stats-throughput! (:stats executor-data) out-stream-id (.reportRate rate-tracker)))
--- End diff --
This is calculating the rate for the spout ignoring the stream-id, and then storing it into the other stats saying it is for a specific stream id. That does not work. Either we have to drop the stream-id from the reported stats or we have to calculate it correctly per stream id.
---
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-1057] Add throughput metrics to spouts/...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144163913
storm-core/src/genthrift.sh permssions should be changed back to -> 644
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40847090
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
* @param count number of arrivals
*/
public void notify(long count) {
- _histograms[_histograms.length-1]+=count;
+ _histograms[_numOfSlides - 1] += count;
--- End diff --
I don't think this is thread safe. I'm not sure that is a big deal for this code, as I am not aware of any spouts that emit from multiple threads ignoring the call to nextTuple. But we should file a JIRA at least to fix it because the disruptor code calls it from multiple threads, and the counts will get off.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150076986
Hi @revans2 ,
Thanks for very much for your time and efforts. Following your suggestions, I have made ```update-values``` as a regular function and fixed the problem in ```storm/STORM-UI-REST-API.md```.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144032486
Hi @HeartSaVioR,
Sorry to interrupt, but could please kindly review the code? I am looking forward to your response. Thanks
---
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-1057] Add throughput metrics to spouts/...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197568939
@wangli1426 given that we've two +1s and one +0. Can you please upmerge this.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42242476
--- Diff: storm-core/src/storm.thrift ---
@@ -279,9 +280,10 @@ struct ComponentAggregateStats {
struct TopologyStats {
1: optional map<string, i64> window_to_emitted;
2: optional map<string, i64> window_to_transferred;
-3: optional map<string, double> window_to_complete_latencies_ms;
-4: optional map<string, i64> window_to_acked;
-5: optional map<string, i64> window_to_failed;
+3: optional map<string, double> window_to_throughput;
+4: optional map<string, double> window_to_complete_latencies_ms;
+5: optional map<string, i64> window_to_acked;
+6: optional map<string, i64> window_to_failed;
--- End diff --
Here too, we cannot renumber the entires or the thrift code will not be compatible with older versions.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148681032
Hi @revans2 ,
I up-merged my code successfully. Following your suggestion, I mark throughput in storm.thrift as optional. Look forward to your response. Thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150751146
@d2r ,
Thank you very much for your prompt response. However, I cannot quite understand your meaning by
> <cite>If the previous worker's throughput stats had declined sharply before the worker had died, then weighting the current worker's throughput stats still would be inaccurate, but in a different way. </cite>
I will appreciate it a lot if you could provide a concrete example.
I couldn't agree with you more than storm needs a History Server keep historical information. Otherwise, executors are responsible for maintaining their stats, which make them stateful. Is there any plan about the history server?
By the way, adding throughput metric is my first step. And my ultimate goal is to add ***normalized*** throughput, which leverages queueing theory to provide a comparable performance metrics, similar but more accurate than ```capacity``` that is currently available in Storm. With normalized throughput, one can easily identify the performance bottleneck of a running topology by finding the executor with minimal number in normalized throughput. With this capability, we can develop a runtime scheduling algorithm to make better resource allocation. So what do you think?
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40846293
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -87,7 +95,7 @@ public final float reportRate() {
sum += _histograms[i];
}
- return sum / (float) duration * 1000;
--- End diff --
Isn't the disruptor queue using RateTracker already? Is this a bug in rate tracker where it said it was doing milliseconds but was really doing seconds? Or is this going to make the disruptor metrics wrong?
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42043550
--- Diff: storm-core/src/storm.thrift ---
@@ -204,6 +203,7 @@ struct ExecutorStats {
2: required map<string, map<string, i64>> transferred;
3: required ExecutorSpecificStats specific;
4: required double rate;
+ 5: required map<string, map<string, double>> throughput;
--- End diff --
This is still not addressed.
---
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 issue #753: [STORM-1057] Add throughput metrics to spouts/bolts and di...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the issue:
https://github.com/apache/storm/pull/753
This new feature has been implemented on master branch in #1719. Please 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150077548
Hi @d2r,
Could you please review this PR? Your response is highly appreciated. Thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42259449
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -205,6 +205,9 @@
[stats stream amt]
(update-executor-stat! stats [:common :transferred] stream (* (stats-rate stats) amt)))
+(defn update-stats-throughput! [stats stream throughput]
+ (update-executor-stat! stats [:common :throughput] stream (* (stats-rate stats) throughput)))
+
--- End diff --
Thanks for the comment, I will delete those lines.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40846514
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -60,6 +65,9 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
assert(_slideSizeInMils > 1);
_histograms = new long[_numOfSlides];
Arrays.fill(_histograms,0L);
+ /* the first instance of RateTracker is responsible for creating this globally shared timer. */
--- End diff --
Can we use c++ style comments `\\`` instead of `\* *\`
---
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-1057] Add throughput metrics to spouts/...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r46511139
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -358,11 +378,13 @@
(defn thriftify-executor-stats
[stats]
(let [specific-stats (thriftify-specific-stats stats)
- rate (:rate stats)]
- (ExecutorStats. (window-set-converter (:emitted stats) str)
- (window-set-converter (:transferred stats) str)
- specific-stats
- rate)))
+ rate (:rate stats)
--- End diff --
indentation
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197118115
@harshach I can up-merge this commit. But before I start, I would like to make sure that more committers want this PR get merged. I really spent a lot of time and effort in up-merging this PR last time.
By the way, should I upmerge this PR for master or only for 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-1057] Add throughput metrics to spouts/...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197367675
> @revans2 @d2r @redsanket can you comment. thanks.
I remember having the concern that the way the throughput stats are presented (e.g., extrapolating when the time window was too short) would result in inconsistency with the other stats presented.
My concern is not serious enough concern to block this from being merged. We can handle issues that appear later as they come up.
+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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40911206
--- Diff: storm-core/src/jvm/backtype/storm/utils/RateTracker.java ---
@@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
* @param count number of arrivals
*/
public void notify(long count) {
- _histograms[_histograms.length-1]+=count;
+ _histograms[_numOfSlides - 1] += count;
--- End diff --
I am glad to hear that.
> On Oct 1, 2015, at 21:09, Robert (Bobby) Evans <no...@github.com> wrote:
>
> In storm-core/src/jvm/backtype/storm/utils/RateTracker.java <https://github.com/apache/storm/pull/753#discussion_r40910516>:
>
> > @@ -72,7 +80,7 @@ public RateTracker(int validTimeWindowInMils, int numOfSlides, boolean simulate
> > * @param count number of arrivals
> > */
> > public void notify(long count) {
> > - _histograms[_histograms.length-1]+=count;
> > + _histograms[_numOfSlides - 1] += count;
> I was planning on doing it today, because I had already pulled it into my distribution, and it should not be that hard to do. It is mostly switching the _histograms to be an array of AtomicLong instead of regular long, and then making sure that the current bucket is modified using getAndSet so that there is no read modify write that is not atomic. I was also thinking I would do something so that the current bucket does not require any offset calculations, but that is a very small optimization, that once the JIT kicks in probably would not make any difference.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/apache/storm/pull/753/files#r40910516>.
>
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-145260599
@revans2 ,
I have addressed all your concerns in d552a99. The most important modification is that instead of employing RateTracker, I reuse the stats of ```emitted``` and ```executed``` to generate the throughput stats for spout and bolt respectively.
---
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-1057] Add throughput metrics to spouts/...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-155489896
> @d2r ,
> Thank you very much for your prompt response. However, I cannot quite understand your meaning by
>
> If the previous worker's throughput stats had declined sharply before the worker had died, then weighting the current worker's throughput stats still would be inaccurate, but in a different way.
>
> I will appreciate it a lot if you could provide a concrete example.
Suppose an executors throughput behaves such that it starts at a normal level, but declines for a while until the worker dies. (This could happen if there is a leak leading to JVM heap exhaustion, leading to frequent major garbage collections and then to the worker being killed.) When the worker restarts, it will restart with an empty metrics state and will begin again at normal throughput. What we would see is a weighted metric that seems to say the executor has had a normal throughput for the entire duration of the window, when in reality the executor's throughput was lower.
The other metrics we use so far reset the count when the new worker is started, and they do not extrapolate for the full time window. So adding a throughput metric that does not behave this way would be inconsistent. The throughput numbers would not correlate with executed and transferred numbers, for example.
This is the inconsistency I was referring to. Both methods can be inaccurate, but they are inconsistent when both are used together. I would rather the UI be self-consistent, for our users' sakes. I would like to avoid mixing metrics that measure what an executor actually has done in the past window and metrics that extrapolate what an executor should have done in the past window.
> I couldn't agree with you more than storm needs a History Server keep historical information. Otherwise, executors are responsible for maintaining their stats, which make them stateful. Is there any plan about the history server?
There was some talk awhile ago around integrating Hadoop's Timeline Server that is used with Tez, but looking in JIRA, I do not see an Issue created for it. This is some thing that would definitely help users.
> By the way, adding throughput metric is my first step. And my ultimate goal is to add normalized throughput, which leverages queueing theory to provide a comparable performance metrics, similar but more accurate than capacity that is currently available in Storm. With normalized throughput, one can easily identify the performance bottleneck of a running topology by finding the executor with minimal number in normalized throughput. With this capability, we can develop a runtime scheduling algorithm to make better resource allocation. So what do you think?
Feedback to the scheduler is a good idea, and it would open up interesting possibilities. The capacity metric tries to show whether an executor is able to keep up with the rate of input, and if that is what you mean by normalized throughput, it could then replace capacity. We would just need to make sure to update the tooltip explaining what the metric means.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144050232
Thank you for your prompt reply. Please review the code when you come back. Wish you have a good time.
> On Sep 29, 2015, at 20:30, Jungtaek Lim <no...@github.com> wrote:
>
> @wangli1426 <https://github.com/wangli1426>
> Sorry to response later.
> We're having holidays in South Korea, 'Chuseok', very similar to 'Mid-autumn festival'.
> It ends just Today, so it'll take a few days to get back.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/apache/storm/pull/753#issuecomment-144044727>.
>
---
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-1057] Add throughput metrics to spouts/...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r46509732
--- Diff: storm-core/src/jvm/backtype/storm/generated/AlreadyAliveException.java ---
@@ -51,7 +51,7 @@
import org.slf4j.LoggerFactory;
@SuppressWarnings({"cast", "rawtypes", "serial", "unchecked"})
-@Generated(value = "Autogenerated by Thrift Compiler (0.9.2)", date = "2015-10-9")
+@Generated(value = "Autogenerated by Thrift Compiler (0.9.2)", date = "2015-10-21")
--- End diff --
I guess thrift 0.9.3 is used now, could you upmerge and generate again
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148394880
Hi @revans2,
Thank you very much for giving so detailed explanation. Your concern is quite reasonable. I will mark the throughput optional. As a recent commit has made substantial modification to stats.clj, I am afraid I need more time to up-merge this PR. I will come back when I am done. Thanks
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42259381
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -626,7 +638,8 @@
(let [^CommonAggregateStats cas (.get_common_stats stats)]
{"stream" stream-id
"emitted" (nil-to-zero (.get_emitted cas))
- "transferred" (nil-to-zero (.get_transferred cas))}))
+ "transferred" (nil-to-zero (.get_transferred cas))
+ "throughput" (float-str (.get_throughput cas))}))
--- End diff --
Don't worry, nil value will be converted into 0 in ```float-str```
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148779028
@revans2 ,
Thanks for your comment. All the concerns are addressed in 99a898f. Please review the code for another round. Thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148720369
It looks better, there are still some issues with the thrift code, as it is not backwards compatible, thrift is hard to get right.
It would be good to update the REST API docs to include throughput now that it is in there.
https://github.com/apache/storm/blob/master/STORM-UI-REST-API.md
Other then that it looks fine.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197654319
@harshach Sure, I am happy to upmerge it now. Should I upmerge it for 1.x-branch, master branch, or 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40845429
--- Diff: storm-core/src/clj/backtype/storm/daemon/executor.clj ---
@@ -550,14 +553,16 @@
has-ackers? (has-ackers? storm-conf)
has-eventloggers? (has-eventloggers? storm-conf)
emitted-count (MutableLong. 0)
- empty-emit-streak (MutableLong. 0)]
-
+ empty-emit-streak (MutableLong. 0)
+ spout-counts (count task-datas)
--- End diff --
spout-counts does not appear to be used anywhere.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148382991
@wangli1426
Thrift classes have two options for member variables. required and optional. If you mark a member as required it must be there or thrift will throw an exception before serializing/deserializing it. This becomes a problem if we want to do a rolling upgrade (upgrade the cluster with no downtime). In that case we upgrade one daemon at a time, and there will be a period of time when old clients are talking to new servers and/or new clients are talking to old servers. If we add new required fields to thrift classes then the code will break during the upgrade. However, if we mark them all as optional and write the code in the client so it does not break if it gets a null for this value, then we will be OK.
I don't want to break a rolling upgrade just so we can have a rate in the UI.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42704498
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -277,15 +277,34 @@
(value-stats stats SPOUT-FIELDS)
{:type :spout}))
+(defn values-divided-by [pairs t]
+ (let [update-values (fn [m f & args]
+ (into {} (for [[k v] m] [k (apply f v args)])))]
+ (update-values pairs / (double t))))
--- End diff --
Thanks for the comment. I will make ```update-values``` as a regular function in this revision.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40848297
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -218,11 +220,17 @@
[stats stream amt]
(update-executor-stat! stats [:common :transferred] stream (* (stats-rate stats) amt)))
+(defn update-stats-throughput! [stats stream throughput]
+ (update-executor-stat! stats [:common :throughput] stream (* (stats-rate stats) throughput)))
+
(defn bolt-execute-tuple!
- [^BoltExecutorStats stats component stream latency-ms]
+ [^BoltExecutorStats stats component stream throughput latency-ms]
(let [key [component stream]]
(update-executor-stat! stats :executed key (stats-rate stats))
- (update-executor-stat! stats :execute-latencies key latency-ms)))
+ (update-executor-stat! stats :execute-latencies key latency-ms)
+ (update-stats-throughput! stats stream throughput)
+ ))
+
--- End diff --
Extra blank line here.
---
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-1057] Add throughput metrics to spouts/...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-201081366
@wangli1426 it would be good to have on both. Thanks.
---
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-1057] Add throughput metrics to spouts/...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197074316
@wangli1426 This is patch very important. I would like to see this get merged in for 1.x-branch. Can you please upmerge this PR.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40846081
--- Diff: storm-core/src/ui/public/js/visualization.js ---
@@ -127,13 +127,13 @@ function renderGraph(elem) {
}
var w = Math.max(55, 25 + gfx.textWidth(node.name));
-
+
--- End diff --
Can we revert this entire file. No reason to change it just for white space differences.
---
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-1057] Add throughput metrics to spouts/...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42704561
--- Diff: STORM-UI-REST-API.md ---
@@ -351,11 +354,13 @@ Sample response:
"executors": 12,
"emitted": 184580,
"transferred": 0,
+ "throughput": "195.000",
"acked": 184640,
"executeLatency": "0.048",
"tasks": 12,
"executed": 184620,
"processLatency": "0.043",
+ "throughput":
--- End diff --
My bad. This will be solved in this revision.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r42673357
--- Diff: storm-core/src/clj/backtype/storm/stats.clj ---
@@ -277,15 +277,34 @@
(value-stats stats SPOUT-FIELDS)
{:type :spout}))
+(defn values-divided-by [pairs t]
+ (let [update-values (fn [m f & args]
+ (into {} (for [[k v] m] [k (apply f v args)])))]
+ (update-values pairs / (double t))))
--- End diff --
I would prefer to see update-values be a regular function instead of defined each time value-divide-by is called.
---
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-1057] Add throughput metrics to spouts/...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150319028
> Hi @d2r,
> Could you please review this PR? Your response is highly appreciated. Thanks.
Firstly, nice job handling the stats.clj upmerge. That is not the nicest code to work with.
My thoughts on this:
The Storm UI does not function like a history server; it only shows us what is currently running. If a worker crashes, then those stats are lost to us.
With the way the current Storm UI works, if a worker is relaunched, then the stats for its executors are presented as if the executors all have been up for the same duration.
[The change](https://github.com/wangli1426/storm/blob/1b0598485adb298d1e008b82ecda706d5996721f/storm-core/src/clj/backtype/storm/stats.clj#L286-L289) weights the throughput rates based upon the time the executor has been running. This could be good, but it has its own problems. If the previous worker's throughput stats had declined sharply before the worker had died, then weighting the current worker's throughput stats still would be inaccurate, but in a different way. Also, the throughput stats would look really confusing compared to the other stats, which are not weighted this way.
Solving the lost stats problem should be part of other work that perhaps creates a History Server for Storm to keep historical information, including some of these lost stats.
It seems to me the current UI is handling it one way (all executors have been up for the same duration) and the patch handles it another (stats weighted by executor's actual uptime). For now, until we get a true history server, I think there is value in keeping the UI self-consistent. I do not think we want to present aggregated throughput numbers that do not correspond to Emitted numbers for a window.
The simple way to do this would be to divide Emitted by the minimum of Topology Uptime & Window. If we did it this way, it would keep the UI consistent, and it could be as simple as changing the JavaScript to do a simple divide in the browser. This way we would not need to change Storm system code or update the thrift serialization.
And as @harshac mentioned, we could add another JavaScript change to show a simple topology-wide throughput in the same way.
What do you think?
---
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-1057] Add throughput metrics to spouts/...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-144044727
@wangli1426
Sorry to response later.
We're having holidays in South Korea, 'Chuseok', very similar to 'Mid-autumn festival'.
It ends just Today, so it'll take a few days to get back.
---
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-1057] Add throughput metrics to spouts/...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150279556
@wangli1426 Thanks for the patch. It looks great. One question why not include throughput at the topology stats . IMO throughput of the entire topology is more important metric for someone to see how the topology is performing.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-148404697
@wangli1426 I totally understand that this is going to take more time. Thank you for your patience.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-154542601
@d2r do you have an answer for @wangli1426's questions?
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/753#discussion_r40845541
--- Diff: storm-core/src/clj/backtype/storm/daemon/executor.clj ---
@@ -663,8 +673,9 @@
(reset! last-active true)
(log-message "Activating spout " component-id ":" (keys task-datas))
(fast-list-iter [^ISpout spout spouts] (.activate spout)))
-
- (fast-list-iter [^ISpout spout spouts] (.nextTuple spout)))
+
+ (fast-list-iter [^ISpout spout spouts] (.nextTuple spout))
--- End diff --
I'm not sure we want to split the ')' onto a new line. It does not follow the formatting.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-150007774
I just have two minor questions now. After that I am +1, but I would like to hear from @d2r. He wrote a lot of the recent metrics code changes and I value his opinion in this area a lot.
---
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-1057] Add throughput metrics to spouts/...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-197349450
I agree with @unsleepy22 +1 except for some unneeded indentation changes (but I can look past them).
The big issue now is that we need a patch for both 1.0 and for 2.0 where the stats code has moved to java.
---
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-1057] Add throughput metrics to spouts/...
Posted by unsleepy22 <gi...@git.apache.org>.
Github user unsleepy22 commented on the pull request:
https://github.com/apache/storm/pull/753#issuecomment-161695575
I'm +1 except for a few indentation nits. Good work!
The throughput metric is kind of similar with RecvTps/SendTps in JStorm, and we can show m1/m5/m15/mean values of every 1min/10min/2h/1d windows.
As for hadoop's timeline server mentioned in the discussion, I'd prefer that there's a metrics server, which stores metrics/historic topology info, etc, and this is what we're currently working on.
---
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.
---