[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-22 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r53723864 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,457 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-22 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r53724182 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,457 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-22 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r53724264 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,457 @@ +/* + * Licensed to

[GitHub] storm pull request: Fix Log4j2.xml config to output the the timest...

2016-02-23 Thread hmcl
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/1145 Fix Log4j2.xml config to output the the timestamp in HH:mm:ss.SSS Currently the -r option outputs the number of milliseconds elapsed from the construction of the layout until the creation of the

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-25 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-188859042 @tgravescs probably there is a bit of miscommunication here. I didn't complete the spout part yet because I have been waiting on getting answers to some foll

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-25 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54144192 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,457 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-25 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-188995178 @revans2 no worries. Concerning your code snippet suggestion, I agree with most of it. We can definitely keep a lot of the state in the MessageId object. I agree it is

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-26 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-189457987 @tgravescs @revans2 I am just finalizing some testing and I will push in the patch after lunch. --- If your project is set up for it, you can reply to this email and

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-02-28 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-189923836 @tgravescs @revans2 I have pushed the latest changes. Please let me know of any feedback or further requirements you may have. Thanks. --- If your project is set up for

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-190868282 @knusbaum can you please let me know how you are trying to run it. I am uploading shortly the changes addressing the code review. So, if you tell me how you are trying to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-190870605 @jianbzhou if tuples 1-5 have been emitted, if 3 fails, currently only 2 is re-emitted. Offsets 1,2 are committed, and the offsets 3-5 are only committed once 2 is acked

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661279 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661265 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661270 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661341 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661283 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661378 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661412 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54661514 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,445 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-190989626 @revans2 I have made enable.auto.commit default to false. --- 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-822: Kafka Spout New Consumer API

2016-03-01 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-191002473 @knusbaum you must either create a uber jar or put kafka-clients/0.9.0.0/kafka-clients-0.9.0.0.jar under extlib --- If your project is set up for it, you can reply

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-02 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54822068 --- Diff: pom.xml --- @@ -836,14 +839,39 @@ test - org.apache.calcite

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-02 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54825460 --- Diff: external/storm-kafka-new-consumer-api/pom.xml --- @@ -0,0 +1,91 @@ + + +http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54905188 --- Diff: external/storm-kafka-new-consumer-api/pom.xml --- @@ -0,0 +1,91 @@ + + +http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r54905145 --- Diff: pom.xml --- @@ -836,14 +839,39 @@ test - org.apache.calcite

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-07 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55189816 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,454 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-07 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55190214 --- Diff: external/storm-kafka-new-consumer-api/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutStream.java --- @@ -0,0 +1,60 @@ +/* + * Licensed

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-07 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55190400 --- Diff: pom.xml --- @@ -836,14 +839,39 @@ test - org.apache.calcite

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-11 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55886553 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaRecordTupleBuilder.java --- @@ -0,0 +1,34 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-11 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55889098 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,458 @@ +/* + * Licensed to the Apache

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-11 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r55889085 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,458 @@ +/* + * Licensed to the Apache

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-14 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r56041205 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutStreams.java --- @@ -0,0 +1,142 @@ +/* + * Licensed to the

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-15 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-197000251 @revans2 @abhishekagarwal87 I was discussing with @harshach and for the condition if it's time to poll or not, I am going to try to impose a byte limit instead of a

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-15 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r56232318 --- Diff: external/storm-kafka-client/pom.xml --- @@ -0,0 +1,86 @@ + + +http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-18 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-197979792 @revans2 @abhishekagarwal87 I am still going over all your comments and I will think through them thoroughly and I am welcome to incorporate everything that makes this

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-19 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r56466552 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaRecordTupleBuilder.java --- @@ -0,0 +1,44 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-19 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r56692645 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java --- @@ -0,0 +1,298 @@ +/* + * Licensed to the

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-19 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r56462751 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java --- @@ -0,0 +1,298 @@ +/* + * Licensed to the

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-25 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/1131#issuecomment-201568283 @harshach @revans2 @abhishekagarwal87 I believe the current implementation addressed everything that we discussed. Can you please take a look and give some feedback

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-29 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r57791718 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,547 @@ +/* + * Licensed to the Apache

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-29 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r57803066 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutRetryService.java --- @@ -0,0 +1,69 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-29 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r57803200 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutRetryService.java --- @@ -0,0 +1,69 @@ +/* + * Licensed to

[GitHub] storm pull request: STORM-822: Kafka Spout New Consumer API

2016-03-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1131#discussion_r57972451 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -0,0 +1,547 @@ +/* + * Licensed to the Apache

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/836#discussion_r43548241 --- Diff: storm-core/src/jvm/backtype/storm/serialization/SerializationFactory.java --- @@ -81,8 +81,9 @@ public static Kryo getKryo(Map conf

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/836#discussion_r43548584 --- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java --- @@ -30,18 +31,20 @@ private HashMap>> bundles = new H

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/836#discussion_r43549300 --- Diff: storm-core/src/jvm/storm/trident/spout/OpaquePartitionedTridentSpoutExecutor.java --- @@ -162,9 +162,9 @@ public void commit(TransactionAttempt

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/836#discussion_r43549404 --- Diff: storm-core/src/jvm/storm/trident/topology/TridentTopologyBuilder.java --- @@ -233,8 +234,9 @@ public StormTopology buildTopology

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/836#discussion_r43549539 --- Diff: storm-core/src/jvm/storm/trident/topology/TridentTopologyBuilder.java --- @@ -248,8 +250,9 @@ public StormTopology buildTopology

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/836#issuecomment-152646032 +1 LGTM. A few pinpoints only related with readability of the code that are left to the discretion of the implementer. --- If your project is set up for it, you can reply

[GitHub] storm pull request: STORM-1152 Change map keySet iteration to entr...

2015-10-30 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/836#issuecomment-152676783 +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 and

[GitHub] storm pull request: STORM-1167: Add windowing support for storm co...

2015-11-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/855#discussion_r45482446 --- Diff: storm-core/src/jvm/backtype/storm/windowing/TupleWindow.java --- @@ -0,0 +1,26 @@ +/** + * Licensed to the Apache Software Foundation (ASF

[GitHub] storm pull request: STORM-1221. Create a common interface for all ...

2015-11-20 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/895#issuecomment-158495768 +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 and

[GitHub] storm pull request: STORM-1221. Create a common interface for all ...

2015-11-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/895#discussion_r45504842 --- Diff: storm-core/src/jvm/storm/trident/TridentTopology.java --- @@ -127,7 +128,21 @@ public Stream newStream(String txId, IPartitionedTridentSpout spout

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46577241 --- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/trident/state/CassandraState.java --- @@ -0,0 +1,149 @@ +/** + * Licensed to

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46577559 --- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/trident/state/CassandraState.java --- @@ -0,0 +1,149 @@ +/** + * Licensed to

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46578459 --- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/trident/state/CassandraState.java --- @@ -0,0 +1,149 @@ +/** + * Licensed to

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46578817 --- Diff: external/storm-cassandra/src/test/java/org/apache/storm/cassandra/DynamicStatementBuilderTest.java --- @@ -116,15 +124,15 @@ private void

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46579539 --- Diff: external/storm-cassandra/src/test/java/org/apache/storm/cassandra/trident/TridentTopologyTest.java --- @@ -0,0 +1,125 @@ +/** + * Licensed to

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46579509 --- Diff: external/storm-cassandra/src/test/java/org/apache/storm/cassandra/trident/TridentTopologyTest.java --- @@ -0,0 +1,125 @@ +/** + * Licensed to

[GitHub] storm pull request: STORM-1211 Added trident support for Cassandra...

2015-12-03 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/915#discussion_r46579684 --- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/trident/state/TridentResultSetValuesMapper.java --- @@ -0,0 +1,63

[GitHub] storm pull request: STORM-1040. SQL support for Storm.

2015-12-03 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/914#issuecomment-161718691 +1. Nice job. --- 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 1179

2015-12-07 Thread hmcl
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/930 Storm 1179 This patch has 3 commits. - POM changes implementing profiles to separate integration tests and unit tests - Two separate commits that illustrate how to migrate existing tests

[GitHub] storm pull request:

2015-12-09 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/commit/99f4494439ef4ab9c8a056ea5742221da08065c8#commitcomment-14881213 In documentation/storm-sql.md: In documentation/storm-sql.md on line 58: `ORDER` shouldn't it be `ORDERS` --- If

[GitHub] storm pull request:

2015-12-09 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/commit/99f4494439ef4ab9c8a056ea5742221da08065c8#commitcomment-14881223 In documentation/storm-sql.md: In documentation/storm-sql.md on line 58: ORDER shouldn't it be ORDERS --- If your pr

[GitHub] storm pull request:

2015-12-09 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/commit/99f4494439ef4ab9c8a056ea5742221da08065c8#commitcomment-14882118 In documentation/storm-sql.md: In documentation/storm-sql.md on line 59: ... clauses regardless of the table being read-only

[GitHub] storm pull request: STORM-1179

2015-12-09 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/pull/930#issuecomment-163363035 Currently, by default no Java or Clojure integration tests get run when executing `mvn clean install`. Only unit tests get executed. The user must run maven with one of the

[GitHub] storm pull request:

2015-12-09 Thread hmcl
Github user hmcl commented on the pull request: https://github.com/apache/storm/commit/99f4494439ef4ab9c8a056ea5742221da08065c8#commitcomment-14882740 In documentation/storm-sql.md: In documentation/storm-sql.md on line 51: Shouldn't the Kafka properties here be fo

[GitHub] storm pull request: [STORM-1175] State store for windowing operati...

2015-12-10 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/939#discussion_r47261403 --- Diff: storm-core/test/jvm/backtype/storm/state/InMemoryKeyValueStateTest.java --- @@ -0,0 +1,73 @@ +/** + * Licensed to the Apache Software

[GitHub] storm pull request #1685: STORM-2097: Improve logging in trident core and ex...

2016-09-15 Thread hmcl
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/1685 STORM-2097: Improve logging in trident core and examples - Improve logging in trident core, MasterBatchCoordinator, and examples - Added DebugMemoryMapState You can merge this pull request into

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-09-16 Thread hmcl
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/1687 Apache master storm 1694 top storm 2097 The Kafka Trident implementation is on top of the Trident logs improvement patch because they are related, and it makes it easier to merge the patch. There is

[GitHub] storm issue #1634: [STORM-2045] fixed SpoutExecutor NPE

2016-09-17 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1634 +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 and wishes so, or if the feature is

[GitHub] storm pull request #1605: STORM-2014: Put logic around dropping messages int...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1605#discussion_r79570505 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutRetryExponentialBackoff.java --- @@ -144,7 +145,12 @@ public String

[GitHub] storm pull request #1605: STORM-2014: Put logic around dropping messages int...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1605#discussion_r79572783 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutRetryService.java --- @@ -29,14 +29,18 @@ */ public

[GitHub] storm pull request #1605: STORM-2014: Put logic around dropping messages int...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1605#discussion_r79571602 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -330,11 +328,9 @@ public void ack(Object messageId

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79576762 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -479,16 +487,17 @@ public OffsetAndMetadata

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79575563 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -277,7 +277,10 @@ private void emit

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79574175 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -266,26 +266,32 @@ private void

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79575910 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -451,11 +454,11 @@ public int compare

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-20 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79576337 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -479,16 +482,17 @@ public OffsetAndMetadata

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-21 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79794331 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -479,16 +482,17 @@ public OffsetAndMetadata

[GitHub] storm pull request #1679: STORM-2087: storm-kafka-client - tuples not always...

2016-09-21 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1679#discussion_r79795823 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -277,7 +277,9 @@ private void emit

[GitHub] storm pull request #1696: STORM-2104: More graceful handling of acked/failed...

2016-09-22 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1696#discussion_r79800937 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/SerializableDeserializer.java --- @@ -0,0 +1,25 @@ +/* + * Copyright

[GitHub] storm pull request #1605: STORM-2014: Put logic around dropping messages int...

2016-09-22 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1605#discussion_r80004187 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java --- @@ -330,11 +328,9 @@ public void ack(Object messageId

[GitHub] storm issue #1605: STORM-2014: Put logic around dropping messages into Retry...

2016-09-22 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1605 I am +1 overall. Please format the commit message to be easy to read and squash the two commits. This is a simple change that should have only one commit. We can merge after that. --- If your project

[GitHub] storm issue #1696: STORM-2104: More graceful handling of acked/failed tuples...

2016-09-22 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1696 @srdo @jfenc91 I am on vacation this week (with limited access to Internet) and I will be back on Monday. Can we please holding on merging this until I can finish my review. I implemented the original

[GitHub] storm issue #1679: STORM-2087: storm-kafka-client - tuples not always being ...

2016-09-22 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1679 @srdo @jfenc91 I am on vacation this week (with limited access to Internet) and I will be back on Monday. **Can we please holding on merging this until I can finish my review on Monday**. I implemented

[GitHub] storm pull request #1691: STORM-2090: Add integration test for storm windowi...

2016-09-28 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1691#discussion_r80987898 --- Diff: integration-test/README.md --- @@ -0,0 +1,59 @@ +End to end storm integration tests +== + +Running

[GitHub] storm issue #1691: STORM-2090: Add integration test for storm windowing

2016-09-28 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1691 @raghavgautam I am +1 overall. I believe it makes sense to have these integration tests run in the mvn integration-test phase, but that's the only detail I have to add. +1 --- If

[GitHub] storm pull request #1691: STORM-2090: Add integration test for storm windowi...

2016-09-28 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1691#discussion_r80988759 --- Diff: integration-test/README.md --- @@ -0,0 +1,59 @@ +End to end storm integration tests +== + +Running

[GitHub] storm issue #1679: STORM-2087: storm-kafka-client - tuples not always being ...

2016-10-27 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1679 @revans2 I will finish my review by lunch time PST --- 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 #1687: Apache master storm 1694 top storm 2097

2016-10-29 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85643212 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/trident/KafkaTridentSpoutEmitter.java --- @@ -0,0 +1,187

[GitHub] storm pull request #1757: Apache master storm 2182 top storm 1694

2016-11-01 Thread hmcl
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/1757 Apache master storm 2182 top storm 1694 This patch complements addresses the code review comments in STORM-1694 concerning the examples. It was the cleanest way to address them was to create its own

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85980640 --- Diff: examples/storm-starter/src/jvm/org/apache/storm/starter/trident/TridentKafkaClientWordCountWildcardTopics.java --- @@ -0,0 +1,51

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85980768 --- Diff: examples/storm-starter/src/jvm/org/apache/storm/starter/trident/DebugMemoryMapState.java --- @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85981903 --- Diff: examples/storm-starter/src/jvm/org/apache/storm/starter/trident/TridentKafkaClientWordCountNamedTopics.java --- @@ -0,0 +1,122

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85983373 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/trident/KafkaTridentSpoutEmitter.java --- @@ -0,0 +1,187

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85983348 --- Diff: examples/storm-starter/src/jvm/org/apache/storm/starter/trident/TridentKafkaClientWordCountNamedTopics.java --- @@ -0,0 +1,122

[GitHub] storm pull request #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on a diff in the pull request: https://github.com/apache/storm/pull/1687#discussion_r85983568 --- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/trident/KafkaTridentSpoutBatchMetadata.java --- @@ -0,0 +1,83

[GitHub] storm issue #1687: Apache master storm 1694 top storm 2097

2016-11-01 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1687 @HeartSaVioR I believe I address your valid concerns about the kafka library versions in this [PR](https://github.com/apache/storm/pull/1757) --- If your project is set up for it, you can reply to

[GitHub] storm issue #1757: Apache master storm 2182 top storm 1694

2016-11-02 Thread hmcl
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1757 @HeartSaVioR why are we trying to avoid the module dependencies so much, to the point of creating duplicate code? Shouldn't we try to avoid creating duplicate code at all costs?

<    1   2   3   4   5   6   7   8   9   >