[GitHub] storm pull request: STORM-1522 should create error worker log loca...

2016-02-10 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1076#issuecomment-182589360 Sounds good +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

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52531803 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -280,7 +281,7 @@ (defn- mk-disruptor-backpressure-handler [executor-data

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52531994 --- Diff: storm-core/src/clj/org/apache/storm/daemon/worker.clj --- @@ -132,7 +132,7 @@ (defn- mk-backpressure-handler [executors] "m

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52531967 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -453,7 +455,7 @@ (let [task-ids (:task-ids executor-data) debug

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52531930 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -302,12 +303,13 @@ ] (disruptor/consume-loop

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52532015 --- Diff: storm-core/src/clj/org/apache/storm/daemon/worker.clj --- @@ -152,7 +152,7 @@ (defn- mk-disruptor-backpressure-handler [worker] "m

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52532053 --- Diff: storm-core/src/clj/org/apache/storm/daemon/worker.clj --- @@ -418,7 +419,7 @@ task->node+port (:cached-task->node+port

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-10 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52532575 --- Diff: storm-core/src/clj/org/apache/storm/disruptor.clj --- @@ -15,75 +15,22 @@ ;; limitations under the License. (ns

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-11 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1095#discussion_r52607318 --- Diff: storm-core/src/clj/org/apache/storm/daemon/executor.clj --- @@ -219,7 +220,7 @@ (let [val (AddressedTuple. task tuple)] (when

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1095#issuecomment-182889339 The changes look good to me and the unit tests all pass. My only concern now is the performance, and the disruptor queue code is on the critical path. Have you run

[GitHub] storm pull request: STORM-1533: IntegerValidator for metric consum...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1091#issuecomment-182892954 +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 this

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1095#issuecomment-183013900 The good news is that this patch doesn't seem to have made the performance on 2.x any worse. The bad news is that it is already 1/2 the throughput I can get o

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1095#issuecomment-183029379 Yes, I used ThroughputvsLatency with a throughput of 20,000 sentences per second that I know 0.10.x can handle. Having a single reflection on the critical path can

[GitHub] storm pull request: [STORM-1532]: Fix readCommandLineOpts to parse...

2016-02-11 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1094#discussion_r52656968 --- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java --- @@ -330,7 +330,11 @@ public static Map readCommandLineOpts() { Map ret = new

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1055#issuecomment-183044681 @redsanket sorry about this, but utils.clj got updated and now this needs to be rebased. It looks like most of the conflicts are fairly trivial. --- If your project

[GitHub] storm pull request: STORM-1272: port backtype.storm.disruptor to j...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1095#issuecomment-183060852 Looks good to me +1 pending Travis --- 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-1248: port backtype.storm.messaging.load...

2016-02-11 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1097#issuecomment-183061460 +1 pending travis --- 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

[GitHub] storm pull request: [STORM-1336] - Evalute/Port JStorm cgroup supp...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1053#discussion_r52767645 --- Diff: conf/defaults.yaml --- @@ -263,7 +263,7 @@ topology.state.checkpoint.interval.ms: 1000 # topology priority describing the importance of the

[GitHub] storm pull request: STROM-1263: port backtype.storm.command.kill-t...

2016-02-12 Thread revans2
GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/1104 STROM-1263: port backtype.storm.command.kill-topology to java (And add in better java CLI) You can merge this pull request into a Git repository by running: $ git pull https://github.com

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52781328 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52781609 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52781890 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52782172 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52782697 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52786150 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52786981 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52789091 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52788861 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r52789245 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,274 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1098#issuecomment-183464954 I didn't realize how messed up timer.clj really was. Besides fixing the locking issues in the code, my biggest suggestion would be to make it fully object oriente

[GitHub] storm pull request: [STORM-1368] change heapdump file permissions ...

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1078#issuecomment-183466471 This breaks windows by default, and is only needed when security is turned on. Why don't we have the logviewer + worker_launcher change the permissions inste

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1055#discussion_r52794040 --- Diff: storm-clojure/pom.xml --- @@ -0,0 +1,83 @@ + + +http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1055#discussion_r52794136 --- Diff: storm-clojure/pom.xml --- @@ -0,0 +1,83 @@ + + +http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1055#discussion_r52795220 --- Diff: storm-clojure/pom.xml --- @@ -0,0 +1,83 @@ + + +http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w

[GitHub] storm pull request: [STORM-1230] port backtype.storm.process-simul...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1100#discussion_r52803469 --- Diff: storm-core/src/jvm/org/apache/storm/ProcessSimulator.java --- @@ -0,0 +1,89 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1230] port backtype.storm.process-simul...

2016-02-12 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1100#discussion_r52803599 --- Diff: storm-core/src/jvm/org/apache/storm/ProcessSimulator.java --- @@ -0,0 +1,80 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1230] port backtype.storm.process-simul...

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1100#issuecomment-183502735 Only two very minor comments, other even without them I am a +1 --- 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-1523] util.clj available-port conversio...

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1073#issuecomment-183509402 +1 lets squash --- 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

[GitHub] storm pull request: STORM-1539 - Improve Storm ACK-ing performance

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1101#issuecomment-183511029 +1 please have a version for 1.x, and please upmerge for 2.x. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] storm pull request: Storm 1539 - Improve Storm ACK-ing performance

2016-02-12 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1105#issuecomment-183515177 +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-1541 Change scope of 'hadoop-minicluster...

2016-02-13 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1102#issuecomment-183755699 +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-1539 - Improve Storm ACK-ing performance

2016-02-13 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1101#issuecomment-183755721 +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: STROM-1263: port backtype.storm.command.kill-t...

2016-02-15 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1104#issuecomment-184398720 The travis failure is unrelated. It is a storm-kafa failure. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] storm pull request: [STORM-1540] Fix Debug/Sampling for Trident

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1112#issuecomment-184703067 +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-1540] Fix Debug/Sampling for Trident

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1112#issuecomment-184703475 Could we look at turning on "topology.testing.always.try.serialize" for the thrift unit tests? This config should have caught it without the need for mul

[GitHub] storm pull request: [STORM-1243] port HealthCheck to java

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/#discussion_r53018855 --- Diff: storm-core/src/jvm/org/apache/storm/command/HealthCheck.java --- @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1243] port HealthCheck to java

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/#discussion_r53019635 --- Diff: storm-core/src/jvm/org/apache/storm/command/HealthCheck.java --- @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1243] port HealthCheck to java

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/#discussion_r53019913 --- Diff: storm-core/src/jvm/org/apache/storm/command/HealthCheck.java --- @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1243] port HealthCheck to java

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/#discussion_r53020251 --- Diff: storm-core/src/jvm/org/apache/storm/command/HealthCheck.java --- @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1243] port HealthCheck to java

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/#issuecomment-184714383 +1 the translation looks really good. I found one minor issue with the original code, and a few very minor style issues. --- If your project is set up for it, you can

[GitHub] storm pull request: [STORM-1543] Always try to reconnect disconnec...

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1103#issuecomment-184780055 +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-1532]: Fix readCommandLineOpts to parse...

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1094#issuecomment-184803373 OK I am +1 for this then. --- 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-1253] - port backtype.storm.timer to ja...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r53052441 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,246 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r53052633 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,246 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r53052812 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,246 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1098#discussion_r53052873 --- Diff: storm-core/src/jvm/org/apache/storm/StormTimer.java --- @@ -0,0 +1,246 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1098#issuecomment-184808041 A few minor comment but overall it looks really good. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1055#discussion_r53055081 --- Diff: storm-core/test/clj/integration/org/apache/storm/integration_test.clj --- @@ -416,23 +452,6 @@ ) ))) -;; (deftest

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-16 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1055#issuecomment-184820877 +1 please rebase and squash commits. --- 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-1245] port backtype.storm.daemon.acker ...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1069#discussion_r53057252 --- Diff: storm-core/src/jvm/org/apache/storm/daemon/AckerBolt.java --- @@ -0,0 +1,128 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1245] port backtype.storm.daemon.acker ...

2016-02-16 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1069#discussion_r53057414 --- Diff: storm-core/src/jvm/org/apache/storm/daemon/AckerBolt.java --- @@ -0,0 +1,129 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1055#discussion_r53335477 --- Diff: storm-clojure/src/test/clj/clojure_test.clj --- @@ -0,0 +1,158 @@ +;; Licensed to the Apache Software Foundation (ASF) under one +;; or more

[GitHub] storm pull request: [STORM - 1258] Backport thrift.clj to Thrift.j...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1055#issuecomment-185795272 I am still +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1098#issuecomment-185798472 +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-1253] - port backtype.storm.timer to ja...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1098#issuecomment-185799741 It looks like there is a minor merge conflict with some imports in two files. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] storm pull request: [STORM-1558] Utils in java breaks component pa...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1119#issuecomment-185800670 +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-hdfs : change visibility of create and c...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1122#issuecomment-185815858 +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 this

[GitHub] storm pull request: [STORM-1253] - port backtype.storm.timer to ja...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1098#issuecomment-185818194 Still +1 I built and ran the tests myself so I am not waiting for travis. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] storm pull request: STORM-1246: port backtype.storm.local-state to...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1117#discussion_r53347252 --- Diff: storm-core/src/clj/org/apache/storm/local_state_converter.clj --- @@ -0,0 +1,25 @@ +;; Licensed to the Apache Software Foundation (ASF) under

[GitHub] storm pull request: STORM-1246: port backtype.storm.local-state to...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1117#discussion_r53348402 --- Diff: storm-core/src/clj/org/apache/storm/daemon/nimbus.clj --- @@ -46,11 +46,11 @@ KillOptions RebalanceOptions ClusterSummary

[GitHub] storm pull request: STORM-1246: port backtype.storm.local-state to...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1117#issuecomment-185827194 A few minor comments about imports but it looks good. +1. Oh and please upmerge or rebase. There are some conflict on imports that are simple to fix. --- If your

[GitHub] storm pull request: [STORM-1532]: Fix readCommandLineOpts to parse...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1115#issuecomment-185828181 +1 here 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

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53354956 --- Diff: storm-core/test/jvm/org/apache/storm/utils/UtilsTest.java --- @@ -0,0 +1,213 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53355813 --- Diff: storm-core/test/jvm/org/apache/storm/utils/UtilsTest.java --- @@ -0,0 +1,213 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53356229 --- Diff: storm-core/test/jvm/org/apache/storm/utils/TimeTest.java --- @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53356278 --- Diff: storm-core/test/jvm/org/apache/storm/utils/TimeTest.java --- @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53356679 --- Diff: storm-core/test/jvm/org/apache/storm/utils/TimeTest.java --- @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1114#discussion_r53357061 --- Diff: storm-core/test/jvm/org/apache/storm/utils/UtilsTest.java --- @@ -0,0 +1,213 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1255: port storm_utils.clj to java and s...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1114#issuecomment-185845912 Just a few comments nothing too big. --- 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-1243] port HealthCheck to java

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/#issuecomment-185849836 +1. The code needs to be upmerged, but it is a minor issue in supervisor.clj --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] storm pull request: [STORM-1553] port event.clj to java

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1110#discussion_r53359219 --- Diff: storm-core/src/jvm/org/apache/storm/event/EventManager.java --- @@ -0,0 +1,26 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1553] port event.clj to java

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1110#issuecomment-185853831 Just one minor request, but I am +1 either way. It would also be nice to file a JIRA to replace this with an ExecutorService. It is probably too complex to do that

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53366140 --- Diff: storm-core/src/clj/org/apache/storm/daemon/supervisor.clj --- @@ -1183,7 +1184,8 @@ (.readBlobTo blob-store (ConfigUtils

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53366400 --- Diff: storm-core/src/clj/org/apache/storm/testing.clj --- @@ -447,8 +447,8 @@ (select-keys component->tasks component-

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53366742 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53366894 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53366988 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53367005 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53368270 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53368387 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53368561 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/IStateStorage.java --- @@ -18,12 +18,14 @@ package org.apache.storm.cluster; import

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53368510 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/ClusterUtils.java --- @@ -0,0 +1,282 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53368693 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/IStateStorage.java --- @@ -18,12 +18,14 @@ package org.apache.storm.cluster; import

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1081#discussion_r53369187 --- Diff: storm-core/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java --- @@ -0,0 +1,687 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-18 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1081#issuecomment-185885496 This is a really big pull request and things are changing all of the time with others merging things in. Perhaps we should break this up into smaller pieces. Like

[GitHub] storm pull request: [STORM-1564] fix wrong package-info in org.apa...

2016-02-19 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1128#issuecomment-186232032 +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-1553] port event.clj to java

2016-02-19 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1110#issuecomment-186233442 @hustfxj integration.org.apache.storm.integration-test failed, but there is no indication in the XML file why it failed. I'll try to take a look. --- If your pr

[GitHub] storm pull request: [STORM-1273] port backtype.storm.cluster to ja...

2016-02-19 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1081#issuecomment-186239627 @hustfxj OK I will try to get through the entire thing today. So we can get it in soon. --- If your project is set up for it, you can reply to this email and have

[GitHub] storm pull request: STORM-561: Add flux as an external module

2016-02-19 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/546#issuecomment-186244256 @gavinfish that is a very difficult thing to do correctly (Which is why we do not currently support it). As part of the JStorm integration work we are looking at adding

[GitHub] storm pull request: [STORM-1553] port event.clj to java

2016-02-19 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1110#issuecomment-186244285 @hustfxj I was not able to reproduce the failure, so I am not sure what happened. The changes look good and all of the tests pass for me so I am +1 --- If your

<    1   2   3   4   5   6   7   8   9   10   >