[GitHub] storm pull request: [STORM-763] nimbus reassigned worker A to anot...

2015-06-03 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/568#issuecomment-108339300 And like Bobby already said: many thanks for your continued work and improvements, @eshioji! --- If your project is set up for it, you can reply to this email and have

[GitHub] storm pull request: [STORM-763] nimbus reassigned worker A to anot...

2015-06-03 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/568#issuecomment-108338846 @eshioji wrote: Also I have a question, maybe @miguno could help; I've removed the graceful shutdown which tries to flush all pending message before the Client

[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

[GitHub] storm pull request: STORM-786: KafkaBolt should ack tick tuples

2015-04-16 Thread miguno
GitHub user miguno opened a pull request: https://github.com/apache/storm/pull/522 STORM-786: KafkaBolt should ack tick tuples [STORM-512](https://issues.apache.org/jira/browse/STORM-512) (KafkaBolt doesn't handle ticks properly) adds special-casing of tick tuples. What is missing

[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-329 : buffer message in client and recon...

2015-03-17 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/268#issuecomment-82217610 @clockfly We may also want to check why we don't have the permissions to close this PR ourselves. --- If your project is set up for it, you can reply to this email

[GitHub] storm pull request: STORM-329 : buffer message in client and recon...

2015-03-16 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/268#issuecomment-81736347 @d2r And btw I don't have permissions on GitHub to close this PR. --- 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-707: Client (Netty): improve logging to ...

2015-03-16 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/463#issuecomment-81739459 I created [STORM-707](https://issues.apache.org/jira/browse/STORM-707) to track this. --- If your project is set up for it, you can reply to this email and have your

[GitHub] storm pull request: Client (Netty): improving logging to help trou...

2015-03-11 Thread miguno
GitHub user miguno opened a pull request: https://github.com/apache/storm/pull/463 Client (Netty): improving logging to help troubleshooting connection woes These logging statements are not on a hot path, and `INFO` is the default log level of Storm. These logging are filling

[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...

2015-03-04 Thread miguno
Github user miguno commented on a diff in the pull request: https://github.com/apache/storm/pull/456#discussion_r25774104 --- Diff: bin/storm --- @@ -183,7 +209,16 @@ def exec_storm_class(klass, jvmtype=-server, jvmopts=[], extrajars=[], args=[] os.spawnvp(os.P_WAIT

[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...

2015-03-04 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/456#issuecomment-77159580 Many thanks for the PR, Kai! However if the topology is submitted with `storm shell`, this cannot track exception because spawnvp seems to return no stdout

[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...

2015-03-04 Thread miguno
Github user miguno commented on a diff in the pull request: https://github.com/apache/storm/pull/456#discussion_r25774112 --- Diff: bin/storm --- @@ -183,7 +209,16 @@ def exec_storm_class(klass, jvmtype=-server, jvmopts=[], extrajars=[], args=[] os.spawnvp(os.P_WAIT

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-24 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-75713928 Oh, Taylor. Could you also update STORM-404 and STORM-510 as appropriate? --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-23 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-75634123 Phew. :-) Thanks for merging, Taylor! On 23.02.2015, at 22:06, P. Taylor Goetz notificati...@github.com wrote: Disregard last message

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-18 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74927415 @nathanmarz Thanks for the detailed feedback on the max-retries issue. As Bobby suggested, would you mind if we decouple the work on max-retries (tracked at STORM-677

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-18 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74894542 FYI: I created [STORM-677: Maximum retries strategy may cause data loss](https://issues.apache.org/jira/browse/STORM-677) to address the issue that Bobby brought up

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-18 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74895041 PS: We may also want to update the original [STORM-329](https://issues.apache.org/jira/browse/STORM-329) ticket description to reflect the changes in this PR

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-18 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74898690 Thanks, Taylor! Let me know if I can help with sorting out the test failures. Also regarding JIRA: I forgot to mention that it looks like we need to update STORM

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-17 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74702215 And FWIW, with the code in this PR the total test suite takes about 5mins to complete. ``` $ mvn clean install ... [INFO

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-17 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74701138 I am seeing a lot of tests timing out with this change. Has anyone else seen this? Hmm. All the tests are passing for me (and they have been since a while

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-16 Thread miguno
Github user miguno commented on a diff in the pull request: https://github.com/apache/storm/pull/429#discussion_r24738502 --- Diff: storm-core/src/jvm/backtype/storm/messaging/netty/Client.java --- @@ -42,344 +42,577 @@ import org.slf4j.LoggerFactory; import

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-12 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74147926 If you need at-least-once processing you must use an acking topology, which will allow Storm to replay lost messages. If instead you go with an unacking topology

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-12 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/429#issuecomment-74032874 This patch allows a worker to properly detect that the connection to a peer becomes unavailable -- for whatever reason (the remote worker is dead or restarting

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-11 Thread miguno
GitHub user miguno opened a pull request: https://github.com/apache/storm/pull/428 STORM-329: fix cascading Storm failure by improving reconnection strategy and buffering messages This is an improved version of the original pull request discussed at https://github.com/apache/storm

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-11 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/428#issuecomment-73946399 I did exactly this in #429. --- 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-329: fix cascading Storm failure by impr...

2015-02-11 Thread miguno
Github user miguno closed the pull request at: https://github.com/apache/storm/pull/428 --- 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

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-11 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/428#issuecomment-73928303 I tried rebasing (also to fix the incorrect commit message that starts with STORM-32*7*) but gave up after several failed attempts. Feel free to give it a try though

[GitHub] storm pull request: STORM-329: fix cascading Storm failure by impr...

2015-02-11 Thread miguno
GitHub user miguno opened a pull request: https://github.com/apache/storm/pull/429 STORM-329: fix cascading Storm failure by improving reconnection strategy and buffering messages **This PR contains the same code as https://github.com/apache/storm/pull/428 but as a single commit

[GitHub] storm pull request: STORM-329 : buffer message in client and recon...

2015-02-10 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/268#issuecomment-73667341 Many thanks for your review, Sean. I addressed your comments, see the new commits in https://github.com/miguno/storm/commits/0.10.0-SNAPSHOT-STORM-392-miguno-merge

[GitHub] storm pull request: STORM-329 : buffer message in client and recon...

2015-02-10 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/268#issuecomment-73671537 PS: I just noticed that I put the wrong Storm ticket into the commit https://github.com/miguno/storm/commit/8ebaaf8dbc63df3c2691e0cc3ac5102af7721ec3. The `STORM-327

[GitHub] storm pull request: STORM-329 : buffer message in client and recon...

2015-02-03 Thread miguno
Github user miguno commented on the pull request: https://github.com/apache/storm/pull/268#issuecomment-72653588 @tedxia (or @clockfly): Have you experienced similar Storm behavior as [I described above](https://github.com/apache/storm/pull/268#issuecomment-72652704) in your patched

[GitHub] storm pull request: STORM-329 : buffer message in client and recon...

2015-02-03 Thread miguno
Github user miguno commented on a diff in the pull request: https://github.com/apache/storm/pull/268#discussion_r24039811 --- Diff: storm-core/src/jvm/backtype/storm/messaging/netty/Client.java --- @@ -142,6 +147,15 @@ public void run