Skip to content
This repository has been archived by the owner on Apr 18, 2018. It is now read-only.

add heartbeat response (#9) #76

Closed
wants to merge 1 commit into from
Closed

add heartbeat response (#9) #76

wants to merge 1 commit into from

Conversation

tdihp
Copy link

@tdihp tdihp commented Dec 9, 2014

Hi, I added heartbeat, so that storm 0.9.3 may work.

@ecanzonieri
Copy link
Contributor

+1
Thanks for contributing

@poros poros added this to the Storm 0.9.3 release milestone Dec 9, 2014
@patricklucas
Copy link
Contributor

@poros, @ecanzonieri: What do you think about putting this logic in Bolt instead of SimpleBolt? Will a user ever want to do anything upon receipt of a heartbeat tuple except sync?

It would be easy for a user to subclass Bolt but forget to check for these tuples and sync. I could be convinced too to make process_tick a part of Bolt as well.

@poros poros added the feature label Dec 9, 2014
@poros
Copy link
Contributor

poros commented Dec 9, 2014

@patricklucas totally agree with you, the Bolt class is the right place for this change.
We can discuss about moving process_tick to Bolt, but I'd keep it separate from this pull request.

@jswetzen
Copy link
Contributor

This failed when I tried it, the ack after a heartbeat sync makes Storm throw an error. Also, because _process_tuple is overridden in SimpleBolt, the heartbeat code will be needed in both of the classes. I don't want to "steal" the commit, but I have a version that works: jswetzen/pyleus@a929e42

@poros
Copy link
Contributor

poros commented Jan 29, 2015

I am up for merging the change as in @jswetzen commit and do not block this on #82. It seems to get the job done and a bit of code replication won't hurt that much if we know we are going to change it in the future.

@jswetzen: in order not to steal @tdihp you can git rebase your work on his branch (or git cherry-pick his commit or stuff like that); otherwise you can just open a new pull request. Whatever works for the two of you is good, guys.

@tdihp
Copy link
Author

tdihp commented Jan 30, 2015

@poros, @jswetzen: I'm okay without commit log.

@poros
Copy link
Contributor

poros commented Jan 30, 2015

Moved to #97

@poros poros closed this Jan 30, 2015
@patricklucas patricklucas modified the milestones: v0.3.0, Storm 0.9.3 release Jun 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants