update to new shutdown semantics #42
Conversation
b008d28 to
b5cc96d
Compare
| if @consumer_group.running? | ||
| @consumer_group.shutdown | ||
| end | ||
| stop | ||
| sleep(Float(@consumer_restart_sleep_ms) * 1 / 1000) |
There was a problem hiding this comment.
I'm wondering what happen here, you added the stop call, but then after it just go into an sleep? would it be possible the thread is waiting here? ... if so, I think it would be necessary to keep track of this so the stop call is able to wakeup the thread from the sleep. What do you think?
There was a problem hiding this comment.
ah, you're right. I shouldn't call stop here, it should be left alone
There was a problem hiding this comment.
This sleep should also be updated to use Stud.stoppable_sleep https://github.com/jordansissel/ruby-stud/blob/master/lib/stud/interval.rb#L76-L93
There was a problem hiding this comment.
switched to using it
|
@talevy leave you two small comments to understand a bit more the context of your changes. Let me know what do you think. |
005c477 to
b76e1fe
Compare
|
updated to reflect your comments, thanks! |
b76e1fe to
1948475
Compare
|
LGTM |
| end | ||
| sleep(Float(@consumer_restart_sleep_ms) * 1 / 1000) | ||
| retry | ||
| Stud.stoppable_sleep(Float(@consumer_restart_sleep_ms) * 1 / 1000) { stop? } |
There was a problem hiding this comment.
iono, I didn't want to edit this. looks like @consumer_restart_sleep_ms is just a number so this forces it to be a float for more precision.
|
LGTM |
|
Merged sucessfully into master! |
Fixes #41.
This depends on elastic/logstash#3895