STORM-312 : add storm monitor tools to monitor throughtput interactively #117
Conversation
update from apache/incubator-storm
update from apache/incubator-storm
update from apache/incubator-storm
update from apache/incubator-storm
|
Sorry this has taken me so long to review. I keep hoping I'll find more time, but never seem to. I really shouldn't review code that I wrote myself, it makes me feel bad. You should find someone else to borrow code from :). |
| import java.util.Map; | ||
|
|
||
| public class Monitor { | ||
| @Option(name="--help", aliases={"-h"}, usage="print help message") |
There was a problem hiding this comment.
Can we separate out the argument parsing from the monitor class. The rest of the code uses clojure.tools.cli and I think it would be more consistent to use that then add in a new dependency. It would also allow the Monitor class to be separated out to be used as a library.
There was a problem hiding this comment.
Sorry for putting you in this bad position. I simply thought that your code could be stepped forward to be a useful monitoring tool. And thanks for your carefully review.
As you say, it is much better to use clojure.tools.cli. I will fix this in time.
…us in mertics; add argument -s stream to specify stream
|
Sorry for putting you in such bad situation. I simply thought your code could be stepped forward to be a useful monitoring tool. And Thanks for your carefully review. |
| @@ -390,6 +390,19 @@ def print_classpath(): | |||
| """ | |||
| print get_classpath([]) | |||
|
|
|||
| def monitor(*args): | |||
| """Syntax: [storm monitor [-i interval-secs] -t topology-name -m component-id [-s stream-id] [-w [emitted | transferred]]*] | |||
There was a problem hiding this comment.
I would prefer to see topology-name be an argument not an option. It matches better with the other commands list kill and rebalance.
I would love it if we could make component-id optional, so that it would do one line per component if not supplied, or if you could do a list of component ids.
And I'm not sure that we want a '*' after the '[emitted | transferred]' to me that means that you can supply 0 or more of them, but the reality is only the last one will be used like stream-id.
There was a problem hiding this comment.
Yep, setting topology-name be an argument better matches the rest commands, and the '*' should be removed.
As for the component-id option, shall we just do a list of component ids if not supplied or wrongly supplied. Moreover, we can do the same for stream-id option.
|
The code changes you made look good. I have a few new comments, also I would like another committer to take a look at the code, as I did kind of write some of it, although you have cleaned it up enough that it no longer looks like my code, which is a good thing. Thanks for putting this together. |
|
Your comments are quite appropriated, Thank you. |
|
Looks good, I am +1, but like I said I would like some others to take a look too. |
|
@miguno Could you spare time to take a look at this please ? |
|
Just an attempt at manners: is there anything wrong with jumping in and making comments as a fellow storm user (novice contributor)? |
|
@danehammer Actually the opposite, it's encouraged |
|
@danehammer , Any comments are quite appreciated. |
| ;; limitations under the License. | ||
| (ns backtype.storm.command.monitor | ||
| (:use [clojure.tools.cli :only [cli]]) | ||
| (:use [backtype.storm thrift config log]) |
There was a problem hiding this comment.
I know we're trying to move to more idiomatic clojure, so here it'd be more readable to specify what you're using (which it looks like you're not using very much from each of these).
There was a problem hiding this comment.
That's right. We are using :only [with-configured-nimbus-connection] from thift.
…tricState and Poller better fits for Java
|
The changes look good to me I am still a +1 |
|
Code looks good. Tried out the different options with word_count, seems to work. |
Fix users auth logic
This cmd line tool 'storm monitor' will use Nimbus.Client to get throughput information from Nimbus Server.
1)One can specify topology's name, component's name to monitor it's throughput interactively.
2) It will statistics 'emit' or 'transferred' throughput in a given time window and print it in a given time frequency
The implementation will be much like yahoo's storm-perf-test (http://yahooeng.tumblr.com/post/64758709722/making-storm-fly-with-netty)