-
Notifications
You must be signed in to change notification settings - Fork 107
add sleep_spout_wait_strategy_time_ms to topology config #135
Conversation
Oh, I see... That was introduced in storm 0.8.1 but we never extended support for this option. Thank you for pointing this out. :) However, you based your change on #134, so, before you can merge, you either need to remove #134's changes from your pull request (try out the |
+1 |
@@ -3,7 +3,8 @@ | |||
# The topology used as example in the Storm tutorial has been implemented with | |||
# Pyleus. https://github.com/nathanmarz/storm/wiki/Tutorial | |||
name: exclamation_topology | |||
|
|||
sleep_spout_wait_strategy_time_ms: 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this example as minimal as possible.
Instead of putting this here, would you mind adding a reference to this option in the documentation. This should go here http://yelp.github.io/pyleus/yaml.html
Thank you for adding more features! Please fix the documentation issue, so we can go ahead and merge. |
hi @poros I've updated the doc and added more features to Pyleus. Enjoy! Let me know if there are more needs Thanks |
@@ -33,18 +33,18 @@ | |||
<dependency> | |||
<groupId>org.apache.storm</groupId> | |||
<artifactId>storm-core</artifactId> | |||
<version>0.9.4</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm cool with the changes, but you bumped Storm and Kafka versions here. This is a big change and deserves a pyleus release and thorough testing. I'm not against it, though.
Storm 0.9.4 seems a pretty safe upgrade (http://storm.apache.org/2015/03/25/storm094-released.html), while Kafka 0.8.2.1 may be a bit more troublesome.
Asking @ecanzonieri or @patricklucas to weigh in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm cool to revert the changes in pom.xml, it is just that my company has this combination in our production and we are not gonna upgrade Storm very soon... So I figured it is best to alter the settings in my environment. Storm 0.9.4 is totally fine and from my experience Kafka 0.8.2.1 has better management ability than 0.8.1. There has not been any broken changes seen in 0.8.2.1 and we are currently using both 0.8.1 and 0.8.2 in production environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up. I'm in favor of bumping the version numbers, it's just that I'd like to have the opinion of another maintainer before merging the change.
@ecanzonieri do you want to weigh in? |
add sleep_spout_wait_strategy_time_ms to topology config
@howardx you are on the right track, under spout options section you have |
sleep spout wait strategy time is quite useful when use kafka spout or similar. When the source queue is empty, we do not want storm cluster run into the crazy looping