[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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: Do I need to ack tick tuples? Since they're system generated I'm think yes but I'm not sure. Nathan's answer: You should ack all tuples. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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.
```
public class KafkaBoltK, V extends BaseRichBolt {
public void execute(Tuple input) {
if (TupleUtils.isTick(input)) {
return; // Do not try to send ticks to Kafka
}
...
}
```
---
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 enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 required. http://mail-archives.apache.org/mod_mbox/storm-user/201411.mbox/%3ccaaogngz1pet0rfkxn+sseo+djy38lkq4e3gw1gbn-wmmqwk...@mail.gmail.com%3E So I took it out. Just to understand: Is ackibg a tick needed? --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 critical. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 is OK. This patch has languished far too long. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 reservations about the patch. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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. This set of commits should really be squashed before committing to the master. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 function be a static helper method. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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) at storm.kafka.bolt.KafkaBolt.execute(KafkaBolt.java:92) at storm.kafka.bolt.KafkaBoltTest.executeWithBrokerDown(KafkaBoltTest.java:145) ``` ``` java.lang.NullPointerException at backtype.storm.tuple.TupleImpl.isTick(TupleImpl.java:218) at storm.kafka.bolt.KafkaBolt.execute(KafkaBolt.java:92) at storm.kafka.bolt.KafkaBoltTest.executeWithoutKey(KafkaBoltTest.java:134) ``` ``` java.lang.NullPointerException at backtype.storm.tuple.TupleImpl.isTick(TupleImpl.java:218) at storm.kafka.bolt.KafkaBolt.execute(KafkaBolt.java:92) at storm.kafka.bolt.KafkaBoltTest.executeWithByteArrayKeyAndMessage(KafkaBoltTest.java:104) ``` ``` java.lang.NullPointerException at backtype.storm.tuple.TupleImpl.isTick(TupleImpl.java:218) at storm.kafka.bolt.KafkaBolt.execute(KafkaBolt.java:92) at storm.kafka.bolt.KafkaBoltTest.executeWithKey(KafkaBoltTest.java:91) ``` --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 this Kafka queue got really upset (lots of deserialisation exceptions) over these ticks. I'll update the example code as you indicated. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: STORM-512 KafkaBolt doesn't handle ticks prope...
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 method in [TupleHelpers](https://github.com/apache/storm/blob/5a460384f4eb90e4e3870f29090e81633aeafe7b/examples/storm-starter/src/jvm/storm/starter/util/TupleHelpers.java#L29-L30) a member method in TupleImpl as you have done, remove TupleHelpers, and then change the example code to use this method. --- 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 enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
