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

add sleep_spout_wait_strategy_time_ms to topology config #135

Merged
merged 18 commits into from
Sep 22, 2015

Conversation

imcom
Copy link
Contributor

@imcom imcom commented Jul 28, 2015

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

@poros
Copy link
Contributor

poros commented Jul 28, 2015

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 pip.conf trick I mentioned in my last comment on that pull request) or to wait for #134 to be merged into master and then rebase.

@poros poros added the feature label Jul 28, 2015
@imcom
Copy link
Contributor Author

imcom commented Jul 29, 2015

hi @poros I tried pip.conf but got no luck... did not really dig into it though. I prefer to wait #134 to be merged first. Thanks for checking out my PR.

@imcom
Copy link
Contributor Author

imcom commented Jul 29, 2015

seems like the #134 will be revoked, so I will remove the code from #134 here

@poros
Copy link
Contributor

poros commented Jul 29, 2015

+1
Thanks for contributing!

@@ -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
Copy link
Contributor

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

@poros
Copy link
Contributor

poros commented Aug 25, 2015

Thank you for adding more features! Please fix the documentation issue, so we can go ahead and merge.

@imcom
Copy link
Contributor Author

imcom commented Sep 1, 2015

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>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@poros
Copy link
Contributor

poros commented Sep 2, 2015

Just noticed that this implements what has been asked in #88.
Close #88

@poros
Copy link
Contributor

poros commented Sep 9, 2015

@ecanzonieri do you want to weigh in?
It's +1 for me

poros added a commit that referenced this pull request Sep 22, 2015
add sleep_spout_wait_strategy_time_ms to topology config
@poros poros merged commit 5d1e48e into Yelp:develop Sep 22, 2015
@imcom
Copy link
Contributor Author

imcom commented May 12, 2016

@howardx you are on the right track, under spout options section you have zk_hosts property, this is where the Kafka lies. Kafka uses ZK for registration so when you set the topic, Storm will use that topic and the ZK servers to find corresponding Kafka brokers. The only problem is with Storm you can not separate Kafka with Storm, they have to share the same ZK cluster .. but with different (better) chroot

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.

3 participants