[STORM-1057] Add throughput metrics to spouts/bolts and display them on web ui

[wangli1426]

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

[wangli1426]

Hi @HeartSaVioR,

Sorry to interrupt, but could please kindly review the code? I am looking forward to your response. Thanks

[HeartSaVioR]

@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.

[wangli1426]

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 notifications@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 #753 (comment).

[jerrypeng]

storm-core/src/genthrift.sh permssions should be changed back to -> 644

[revans2]

spout-counts does not appear to be used anywhere.

[revans2]

@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.

[revans2]

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.

[wangli1426]

@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.

[wangli1426]

@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.

Look forward to your response. Thanks

[revans2]

@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.

[wangli1426]

@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.

[revans2]

@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.

[wangli1426]

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

[revans2]

@wangli1426 I totally understand that this is going to take more time. Thank you for your patience.

[wangli1426]

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.

[revans2]

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.

[wangli1426]

Thanks for the comment and point taken.

[wangli1426]

@harshach I managed to upmerge this PR to 1.x-branch in #1703. Please review. Thanks.

[wangli1426]

This new feature has been implemented on master branch in #1719. Please review.