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 to Bolt and SimpleBolt (#9) #97

Closed jswetzen closed 9 years ago

jswetzen commented 9 years ago

As @tdihp was okay with it, here is his code with some fixes that adds heartbeat to both Bolt and SimpleBolt but without ack afterwards.

poros commented 9 years ago

Would you mind adding some unit tests to cover your changes (you are free to modify existing ones, if you believe it is the case)? tests/storm/bolt_test.py is the right file for that.

jswetzen commented 9 years ago

The test for SimpleBolt was simple to write but Bolt was harder to write beautifully as there is no fixture there. The alternatives I thought of were:

  1. Writing test_heartbeat much like test_ack and only assert that "sync" is called.
  2. "assert not" process_tuple and ack as well, but with two more "where ... as ..." statements.
  3. Add a fixture much like SimpleBolt to Bolt, but it would only be used for this one function.
  4. Add "fixture code" but only to test_heartbeat. This is what I chose because it seemed nicer than three nested "where ... as ..." statements. I appreciate comments to make this ready for a merge, Thanks.
jswetzen commented 9 years ago

That worked well. It's not longer than the nested wheres would be but to me it's easier to read.

poros commented 9 years ago

+1 Looks good to me I believe that this change is backward compatible, so we can merge it even if pyleus is not Storm 0.9.3 ready. Edit: I tried both locally and remotely the word_count topology with Storm 0.9.2 and it worked fine.

ecanzonieri commented 9 years ago

+1 going to merge this one