search

Yelp / pyleus

Pyleus is a Python framework for developing and launching Storm topologies.
Apache License 2.0
404 stars 113 forks source link

add heartbeat response (#9) #76

Closed tdihp closed 9 years ago

tdihp commented 9 years ago

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

ecanzonieri commented 9 years ago

+1 Thanks for contributing

patricklucas commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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@a929e425cd86d6f9cca87c6b5d37664184f19d99

poros commented 9 years ago

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 commented 9 years ago

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

poros commented 9 years ago

Moved to #97