[GitHub] storm pull request: Storm 509. Make groups checking specific for S...

2014-09-30 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/273#discussion_r18237335 --- Diff: storm-core/src/jvm/backtype/storm/security/auth/authorizer/SimpleACLAuthorizer.java --- @@ -107,12 +108,18 @@ public boolean permit(ReqContext

[GitHub] storm pull request: Storm 509. Make groups checking specific for S...

2014-09-30 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/273#issuecomment-57360464 Once your remove the debug logging I too am +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

[GitHub] storm pull request: STORM-499

2014-09-30 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/274#issuecomment-57361560 +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

[GitHub] storm pull request: STORM-386 nodejs multilang protocol implementa...

2014-09-30 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/177#issuecomment-57363269 @itaifrenkel and @anyatch both of you should be in README.markdown now. Thanks again for your contribution. --- If your project is set up for it, you can reply

[GitHub] storm pull request: [STORM-529] Send fail(tup) when BasicBolt proc...

2014-10-15 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/291#issuecomment-59222813 @itaifrenkel Sorry for the long silence. We have been battling a fire here with all hands on deck. I don't see much of a reason for this to wait on STORM-528. You made

[GitHub] storm pull request: [STORM-529] Send fail(tup) when BasicBolt proc...

2014-10-15 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/291#issuecomment-59226497 I am +1 for this change. The code looks to do as expected, and everything runs well. --- If your project is set up for it, you can reply to this email and have your

[GitHub] storm pull request: [STORM-533] Added in client and server IConnec...

2014-10-24 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/302 [STORM-533] Added in client and server IConnection metrics. You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm iconn

[GitHub] storm pull request: STORM-487 Let bin/storm compatible with Window...

2014-11-06 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/280#issuecomment-61983313 Sorry it took me so long to respond. The changes look good to me, but I don't have a windows box so I cannot test the windows support, I am +1 on the changes so far

[GitHub] storm pull request: [STORM-533] Added in client and server IConnec...

2014-11-06 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/302#issuecomment-61989690 The metrics system is very generic, and not that complex. Essentially it sets up a timer that will periodically call getValueAndReset on an instance of IMetric

[GitHub] storm pull request: STORM-216: Added Authentication and Authorizat...

2014-11-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/121#issuecomment-62754593 I think we are at the point where we should merge the security branch into master. This is a little bit different from most patches/pull requests we have done, partly

[GitHub] storm pull request: STORM-216: Added Authentication and Authorizat...

2014-11-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/121#issuecomment-62782715 Like I said in the mailing list, I have been out for a few weeks. I didn't see 0.9.3 go out, but I thought that it must be very close by now. I'll send a separate mail

[GitHub] storm pull request: STORM-216: Added Authentication and Authorizat...

2014-11-13 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/121#issuecomment-62944630 I see a 0.9.3-branch already out in the git repo. I assume that this is good enough. I will merge in the security work but just to be sure I'll tag the repo right

[GitHub] storm pull request: [STORM-533] Added in client and server IConnec...

2014-11-14 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/302#issuecomment-63134572 @clockfly is it OK to merge this code in? I am happy to file a new JIRA for rethinking how we do metrics in storm, if you think that would be helpful. --- If your

[GitHub] storm pull request: STORM-558 change swap! to reset! to fix as...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/315#issuecomment-63326875 +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

[GitHub] storm pull request: [STORM-557] Created docs directory and added i...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/314#issuecomment-63332256 I was curious if we wanted to keep a 5MB file in our github repo that everyone must download to get storm? As it stands now the git repo is about 11MB total. This could

[GitHub] storm pull request: [STORM-557] Created docs directory and added i...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/314#issuecomment-63337668 OK so the next question I have is, should we split the one big SVG file up along side rasterized PNG equivalents for use in the docs? --- If your project is set up

[GitHub] storm pull request: [STORM-533] Added in client and server IConnec...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/302#issuecomment-63340062 Thanks for the comments on this. I merged it into master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] storm pull request: STORM-532:Supervisor should restart worker imm...

2014-11-17 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/296#discussion_r20450306 --- Diff: storm-core/src/clj/backtype/storm/util.clj --- @@ -372,6 +372,13 @@ (throw (RuntimeException. (str Got unexpected process name: name

[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-63347222 The changes look fine to me. +1, @d2r in this case the bolt is taking its input and sending it to a kafka queue. I don't think we want to send ticks to the kafka queue

[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-63349586 OK I am wrong. When I tried to compile with this change the following tests failed. ``` Failed tests: shouldEmitSomethingIfTickTupleIsReceived

[GitHub] storm pull request: [STORM-557] Created docs directory and added i...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/314#issuecomment-63354593 OK I split them apart, cleaned up one of the SVG images to make it smaller, and added in some PNG versions, along with updating the security document to use one

[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...

2014-11-17 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-63355950 That is what I expected, thanks for clarifying @nielsbasjes Now I am very confused why there is a test that looks like it explicitly tests that the ticks are persisted

[GitHub] storm pull request: [STORM-410] Add groups support to log-viewer

2014-11-18 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/317 [STORM-410] Add groups support to log-viewer You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-410 Alternatively

[GitHub] storm pull request: STORM-430: Allow netty SASL to support encrypt...

2014-11-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/250#issuecomment-63491318 ping? I really would like to try and get 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

[GitHub] storm pull request: STORM-188: Allow user to specifiy full configu...

2014-11-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/120#discussion_r20513760 --- Diff: storm-core/src/jvm/backtype/storm/utils/Utils.java --- @@ -122,36 +124,64 @@ public static void sleep(long millis

[GitHub] storm pull request: STORM-188: Allow user to specifiy full configu...

2014-11-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/120#issuecomment-63492543 I just had one comment, to avoid YAML doing some potentially unsafe things, but it is not that critical, because placing a --config on the command line is going to use

[GitHub] storm pull request: STORM-550:fix bug get an error when use --conf...

2014-11-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/310#issuecomment-63493192 This is a duplicate of STORM-188 as @clockfly suggested. This patch has a regression, in that it does not search the classpath for the --config option. I don't expect

[GitHub] storm pull request: STORM-188: Allow user to specifiy full configu...

2014-11-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/120#discussion_r20514725 --- Diff: storm-core/src/jvm/backtype/storm/utils/Utils.java --- @@ -122,36 +124,64 @@ public static void sleep(long millis

[GitHub] storm pull request: STORM-552:add new config storm.messaging.netty...

2014-11-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/311#issuecomment-63495857 @ptgoetz I traced this down in the netty code to the second parameter to the call to bind in the socket. https://docs.oracle.com/javase/7/docs/api/java/net

[GitHub] storm pull request: STORM-430: Allow netty SASL to support encrypt...

2014-11-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/250#issuecomment-63524216 Not a problem it is great work, I would like to see it in. Also note that security has been merged into master so if you could update your pull request to master

[GitHub] storm pull request: [STORM-570] replace table sorter with data tab...

2014-11-25 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/328 [STORM-570] replace table sorter with data table. Also upraded several ... ...javascript libraries to latest versions to make integration simpler. I know this is kind of a lot. I upgraded

[GitHub] storm pull request: [STORM-570] replace table sorter with data tab...

2014-11-26 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/328#issuecomment-64690420 Thanks for the advice. I switched the style over to be compact for the tables. I also fixed an issue with they layout on fire fox where the table always filled more than

[GitHub] storm pull request: [STORM-572] add favicon

2014-11-26 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/326#issuecomment-64700524 +1 sweet! @Parth-Brahmbhatt we don't the apache icon. We can add apache elsewhere on the UI if we feel a need to later, but there are no branding requirements

[GitHub] storm pull request: STORM-567: Move Storm Documentation/Website fr...

2014-11-26 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/330#issuecomment-64707784 For the most part it looks great. It looks like we could use some license headers in some of the files. My only other comment would be to add

[GitHub] storm pull request: [STORM-575] Specify UI/Jetty interface to bind...

2014-11-26 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/329#issuecomment-64722744 This looks fine to me +1. My only concern is that this pull request is against 0.9.3-branch and not master. I will leave it up to @ptgoetz if he wants to have the 0.9.3

[GitHub] storm pull request: [STORM-573]Allow users to pass TEST-TIMEOUT-MS...

2014-11-26 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/327#issuecomment-64723421 Looks fine to me too +1. @javrasya Since STORM-573 is already resolved could you please file another JIRA to track this change separately? --- If your project is set up

[GitHub] storm pull request: [STORM-573]Allow users to pass TEST-TIMEOUT-MS...

2014-11-28 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/327#issuecomment-64931214 @javrasya We typically will resolve the JIRA when it is merged into a release branch, a.k.a master. When the release goes out then we close the JIRA. Thanks

[GitHub] storm pull request: STORM-571. upgrade clj-time.

2014-11-28 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/325#issuecomment-64931782 +1 looks fine to me. I'll merge 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

[GitHub] storm pull request: [STORM-569] Add Configuration to enable/disabl...

2014-11-28 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/324#issuecomment-64932492 Actually the default in the code is false, not true. But it looks fine to me. +1 I'll merge this into master. --- If your project is set up for it, you can reply

[GitHub] storm pull request: use boolean to replace some?, backtype.storm.u...

2014-11-28 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/285#issuecomment-64933603 I like the change, but supervisor_test.clj needs to be updated too (same chage as before). It would also be good if you could file a JIRA for this at http

[GitHub] storm pull request: Typo in storm-kafka README

2014-11-29 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/320#issuecomment-64961892 +1 - this is small enough I am happy to check this in without a JIRA. @Lewuathe in the future feel free to file a JIRA at http://issues.apache.org/jira

[GitHub] storm pull request: Update cluster.xml

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/337#issuecomment-6573 +1 this change itself looks good, but there are a number of issues with the metrics.log file. I filed STORM-584 to address them. --- If your project is set up

[GitHub] storm pull request: Initiali check-in for storm-eventhubs.

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/336#issuecomment-65800546 @shanyu This is a ton of code. Do you have a JIRA for this? Do you have a design document of some sort describing how this works? --- If your project is set up

[GitHub] storm pull request: typo

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/333#issuecomment-65800741 +1 looks fine to me. --- 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

[GitHub] storm pull request: STORM-577´╝Üsupervisor heartbeat and handle ev...

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/332#issuecomment-65801126 The change looks fine to me +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

[GitHub] storm pull request: STORM-567: Move Storm Documentation/Website fr...

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/330#issuecomment-65801181 @ptgoetz any update on 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

[GitHub] storm pull request: [STORM-557] Created docs directory and added i...

2014-12-05 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/314#issuecomment-65801449 With pull #330 now up, I will wait for it to go in, and then rebase my changes on top of it. --- If your project is set up for it, you can reply to this email and have

[GitHub] storm pull request: use boolean to replace some?, backtype.storm.u...

2014-12-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/285#issuecomment-66633105 @taojoe It really doesn't matter that much if it is an improvement or a bug. If you want to file it as a bug that would be fine. After filing the JIRA please add

[GitHub] storm pull request: [STORM-557] Created docs directory and added i...

2014-12-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/314#issuecomment-67888564 Enough changed with the docs update, that I opted to just add the images in. If we want to move some of the other markdown documents over we can, but for now I just left

[GitHub] storm pull request: [STORM-410] Add groups support to log-viewer

2014-12-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/317#issuecomment-67890661 I upmerged and updated SECURITY.md to reflect the appropriate changes. Please take a look again. --- If your project is set up for it, you can reply to this email

[GitHub] storm pull request: [STORM-570] replace table sorter with data tab...

2014-12-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/328#issuecomment-67892015 I just upmerged. --- 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

[GitHub] storm pull request: Storm-444: Moving AutoHDFS to storm-hdfs and A...

2014-12-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/344#issuecomment-67895275 It looks fine to me +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

[GitHub] storm pull request: STORM-600: Update version of jacoco plugin.

2014-12-22 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/357 STORM-600: Update version of jacoco plugin. You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-600 Alternatively you

[GitHub] storm pull request: STORM-601: Make jira-github-join ignore case.

2014-12-23 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/358 STORM-601: Make jira-github-join ignore case. You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-601 Alternatively

[GitHub] storm pull request: STORM-544 Fix outdated documents

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/340#issuecomment-67962843 +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

[GitHub] storm pull request: Storm-456 cannot navigate to topology page whe...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/318#issuecomment-67976452 The issue appears to be that when we url decode the topology name the '+' is staying as a '+' if I replace it with a space ' ' in the URL it seems to work for me on both

[GitHub] storm pull request: STORM-552:add new config storm.messaging.netty...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/311#issuecomment-67976914 @caofangkun any update on 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

[GitHub] storm pull request: STORM-505: Fix debug string construction

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/359#issuecomment-67979768 +1 this is a rebase of https://github.com/apache/storm/pull/266 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] storm pull request: STORM-248 STORM-322 proposal for fix

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/353#issuecomment-67982494 +1 the changes look fine to me. Please upmerge. the storm python script has changed since you did the pull request and there are some minor merge conflicts that should

[GitHub] storm pull request: STORM-243,Record version and revision informat...

2014-12-23 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/294#discussion_r8158 --- Diff: storm-core/src/clj/backtype/storm/ui/core.clj --- @@ -52,15 +52,9 @@ (map #(.get_stats ^ExecutorSummary %)) (filter not-nil

[GitHub] storm pull request: STORM-243,Record version and revision informat...

2014-12-23 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/294#discussion_r8357 --- Diff: storm-dist/binary/src/main/assembly/binary.xml --- @@ -131,13 +131,6 @@ /file !-- TODO this should be a generated file from

[GitHub] storm pull request: STORM-243,Record version and revision informat...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/294#issuecomment-67985049 @caofangkun thank you for submitting this patch. I appreciate you trying to help. In the future it would be good though, to be sure that your patch complies

[GitHub] storm pull request: [STORM-442] multilang ShellBolt/ShellSpout die...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-67989780 I see this has not had that much traffic in quite a while. The code to me looks acceptable. I am not really happy with calling .available() twice, but I can live

[GitHub] storm pull request: [STORM-591] exclude logback.xml from jar

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/352#issuecomment-67997538 This seems to be used by the tests, after this change see a lot of DEBUG and INFO logs from zookeeper. Instead of deleting it, can we move it to a directory that is only

[GitHub] storm pull request: [STORM-599] Use use nimbus's cached heartbeats...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/356#issuecomment-67997939 The changes look fine to me. My only concern would be around what happens if the cache has not been updated and is empty, but it looks like it will return an empty map

[GitHub] storm pull request: STORM-598:Newly submitted topologies do not sh...

2014-12-23 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/355#discussion_r22236169 --- Diff: storm-core/src/clj/backtype/storm/daemon/nimbus.clj --- @@ -1248,7 +1256,7 @@ (when-let [owner

[GitHub] storm pull request: STORM-598:Newly submitted topologies do not sh...

2014-12-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/355#issuecomment-68001550 Also I tried to reproduce the issue on master and I am seeing the second topology come up on the ui without any issues. Could you try to reproduce the error on master

[GitHub] storm pull request: STORM-581. Add rebalance params to Storm REST ...

2015-01-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/370#issuecomment-71254799 +1 the code looks good. It needs to be upmerged, but other then that it looks fine. --- If your project is set up for it, you can reply to this email and have your

[GitHub] storm pull request: STORM-635. logviewer returns 404 if storm_home...

2015-01-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/391#issuecomment-71253695 +1 looks good to me. --- 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

[GitHub] storm pull request: [STORM-629] Place Link to Source Code Reposito...

2015-01-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/386#issuecomment-71263897 +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

[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external

2015-01-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-71273044 +1 the changes still look good Are there any other committers that want to take a look at the code? I would feel more comfortable with more people looking

[GitHub] storm pull request: STORM-598:Newly submitted topologies do not sh...

2015-01-23 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/355#issuecomment-71263582 The code compiles now, but I still have not been able to reproduce the original cause of this problem. am I doing something wrong? ``` ./bin/storm dev-zookeeper

[GitHub] storm pull request: Storm 528

2015-02-02 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/389#issuecomment-72539951 There is a lot of code in here that I don't understand how it relates to STORM-528. Setting the code dir, is not really something that I think we want to expose, except

[GitHub] storm pull request: update worker.clj-delete missing-tasks check...

2015-02-02 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/288#issuecomment-72536306 @BuDongDong I merged this in, but because your repo was deleted the pull request is not automatically recognizing that it was pulled in. Could you check master and close

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24043926 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/bolt/JdbcBolt.java --- @@ -0,0 +1,81 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24044371 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/common/Column.java --- @@ -0,0 +1,87 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24015372 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/common/Column.java --- @@ -0,0 +1,87 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24014941 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/bolt/JdbcBolt.java --- @@ -0,0 +1,81 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/374#issuecomment-72685012 For the most part it looks OK. I am concerned about a few of the APIs seem confusing to me and could use some better javadocs. Also I am concerned with the performance

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24014635 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/bolt/JdbcBolt.java --- @@ -0,0 +1,81 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24014770 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/bolt/JdbcBolt.java --- @@ -0,0 +1,91 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24015210 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/bolt/JdbcLookupBolt.java --- @@ -0,0 +1,86 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request: [STORM-570] replace table sorter with data tab...

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/328#issuecomment-72688638 I rebased it so it would merge cleanly. @kishorvpatil if you could take a look again that would be great. --- If your project is set up for it, you can reply

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-03 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/374#discussion_r24016216 --- Diff: external/storm-jdbc/src/main/java/org/apache/storm/jdbc/common/JdbcClient.java --- @@ -0,0 +1,213 @@ +/** + * Licensed to the Apache

[GitHub] storm pull request: STORM-534:Store Nimbus Server Information in z...

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/394#issuecomment-72693402 @caofangkun If you want to reopen this and just add in the nimbus version information I would support that. --- If your project is set up for it, you can reply

[GitHub] storm pull request: STORM-534:Store Nimbus Server Information in z...

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/394#issuecomment-72693203 Sorry this took me so long to respond to. I am kind of swamped :). I do like the idea of being able to access the version numbers for nimbus, but also

[GitHub] storm pull request: STORM-350: LMAX Disruptor 3.2.1

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/134#issuecomment-72693694 @darionyaphet, if 3.3.0 is out with a different wait strategy it would at least be worth doing a comparison. Do you have the time to do that? --- If your project

[GitHub] storm pull request: STORM-608. Storm UI CSRF escape characters not...

2015-02-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/371#issuecomment-72717689 +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

[GitHub] storm pull request: [STORM-400] Thrift upgrade to thrift-0.9.2

2015-02-06 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/410#issuecomment-73267260 The changes look good for the most part, but I am seeing a hang that comes up rather frequently. ``` main #1 prio=5 os_prio=31 tid=0x7fc339801000 nid

[GitHub] storm pull request: [STORM-624] Fix typos in SECUTIRY.md

2015-01-14 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/379#issuecomment-69969297 +1, I didn't realize that I forgot to spell check it when I first wrote it. Thanks @Lewuathe --- If your project is set up for it, you can reply to this email and have

[GitHub] storm pull request: STORM-354: Testing: allow users to pass TEST-T...

2015-01-20 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/122#issuecomment-70688283 https://github.com/apache/storm/blob/master/storm-core/src/clj/backtype/storm/testing.clj#L190-192 Shows that the environment variable STORM_TEST_TIMEOUT_MS, can

[GitHub] storm pull request: STORM-354: Testing: allow users to pass TEST-T...

2015-01-20 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/122#issuecomment-70690787 So it is. I guess I should read the documentation before I start digging though the code :) Thanks @harshach --- If your project is set up for it, you can reply

[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external

2015-01-20 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23232733 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/trident/state/RedisClusterMapState.java --- @@ -0,0 +1,295 @@ +/** + * Licensed

[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external

2015-01-20 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23232475 --- Diff: external/storm-redis/pom.xml --- @@ -0,0 +1,108 @@ +?xml version=1.0 encoding=UTF-8? +!-- + Licensed to the Apache Software Foundation

[GitHub] storm pull request: STORM-626: Add script to print out the merge c...

2015-01-14 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/380 STORM-626: Add script to print out the merge command for a given pull request. You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2

[GitHub] storm pull request: [Storm 599] Use use nimbus's cached heartbeats...

2015-01-14 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/381#issuecomment-69963830 +1 I manually brought up the topology and it is behaving as expected. The metrics are updated every 10 seconds like clockwork. --- If your project is set up for it, you

[GitHub] storm pull request: STORM-635. logviewer returns 404 if storm_home...

2015-01-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/391#issuecomment-71058840 The change to check the parent directory is the root dir was put in on purpose for security reasons. if someone puts in a file called ../../../etc/passwd there could

[GitHub] storm pull request: [STORM-630] Support for Clojure 1.6.0

2015-01-22 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/390#issuecomment-71053295 +1 the code looks fine to me. --- 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

[GitHub] storm pull request: Storm-616 : Storm-jdbc connector.

2015-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/374#issuecomment-73911162 I am +1 for merging this in now. But I am not a DB expert so I would feel more comfortable with others giving it a +1 too before going forward. --- If your project

[GitHub] storm pull request: STORM-634: Storm serialization changed to thri...

2015-02-11 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/414#discussion_r24513164 --- Diff: storm-core/src/jvm/backtype/storm/serialization/ThriftSerializationDelegate.java --- @@ -0,0 +1,52 @@ +/** + * Licensed to the Apache

  1   2   3   4   5   6   7   8   9   10   >