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

2015-04-17 Thread nathanmarz
Github user nathanmarz commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-94068063 I didn't get a notification that you were pinging me – but fwiw I'm +1 on it --- If your project is set up for it, you can reply to this email and have your reply

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

2015-04-16 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-93694857 I created [STORM-786](https://issues.apache.org/jira/browse/STORM-786) to track the tick tuple acking. Pull request is already sent. --- If your project is set up for

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

2015-04-16 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-93699373 My understanding is yes, you do need to ack tick tuples. See @nathanmarz [comment](https://groups.google.com/forum/#!topic/storm-user/ZEJabXT5nQA) from some time back:

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

2015-04-16 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-93690832 Since KafkaBolt is extending BaseRichBolt, I think we should perform a `collector.ack(input)` before `return`. Tick tuples must be acked like normal tuples. ```

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

2015-04-16 Thread nielsbasjes
Github user nielsbasjes commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-93697965 I had this ack for the tick in there in an earlier version of the change. Then I asked on the mailing list if this ack is needed and I was told that it is not

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

2015-04-03 Thread asfgit
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/275 --- 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: STORM-512 KafkaBolt doesn't handle ticks prope...

2015-04-03 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-89306290 @nathanmarz I merged this in, but if you feel that there is something wrong with it still I am happy to adjust it on a follow on JIRA, or revert it if it is truly

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

2015-04-02 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-89020139 @nathanmarz any update on your opinion of this. Your original concerns were addressed, and I feel we should check this in. If I don't hear back soon I'll assume that it

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

2015-02-17 Thread nielsbasjes
Github user nielsbasjes commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-74696542 @nathanmarz / @revans2 What must I do/change to get this solution committed? --- If your project is set up for it, you can reply to this email and have your reply

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

2015-01-09 Thread revans2
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-69375242 The changes look find to me I am +1 for merging this in. @nathanmarz I would like to get your opinion on this before merging it in, because you had the original

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

2014-12-15 Thread nielsbasjes
Github user nielsbasjes commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-66971975 @nathanmarz Thanks I understand your view now. I refactored my patch to meet this requirement and I've tried to make it as small a difference as possible.

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

2014-12-12 Thread nathanmarz
Github user nathanmarz commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-66858418 I view tick tuples as being built on top of the core ideas of streams and tuples, not as fundamentally intertwined with them. So let's keep them separate and have this

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

2014-12-11 Thread d2r
Github user d2r commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-6316 @nielsbasjes Yeah that fixed it. I see all tests passing now. Thank you! +1 --- If your project is set up for it, you can reply to this email and have your reply appear on

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

2014-12-09 Thread d2r
Github user d2r commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-66294912 The explanation makes sense. I still see some test errors: ``` java.lang.NullPointerException at backtype.storm.tuple.TupleImpl.isTick(TupleImpl.java:218)

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

2014-10-29 Thread nielsbasjes
Github user nielsbasjes commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-60935570 The execute of the KafkaBolt puts the tuple into Kafka as a message. What I ran into is that the Ticks were put into Kafka as messages too. My consumers of

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

2014-10-28 Thread d2r
Github user d2r commented on the pull request: https://github.com/apache/storm/pull/275#issuecomment-60838468 Looks reasonable to me, but I would like someone more familiar with kafka to comment on whether we never want to execute on tick tuples. We could move make the static