search

wistia / nsq-ruby

NSQ Ruby client
MIT License
68 stars 27 forks source link

Support DPUB to send deferred messages #23

Closed Soulou closed 8 years ago

ghost commented 8 years ago

@Soulou, thanks for your PR! By analyzing the blame information on this pull request, we identified @bschwartz and @freerobby to be potential reviewers

bschwartz commented 8 years ago

Hi Soulou! Thanks for the PR! It looks great, and thanks for updating the README too. A few questions:

What do you think about making deferred_write take the number of seconds to defer rather than a time? That feels like it will be more in line with how it is in NSQ and how it is in the reference client implementations:

Also, should it take seconds or milliseconds as its argument? Feeling divided on that. Seconds seems more Ruby-esque I suppose.

Any thoughts @freerobby?

bschwartz commented 8 years ago

@Soulou any thoughts on using seconds as an arg instead of a time for deferred_write? And thoughts on using seconds instead of milliseconds to be more ruby-like (e.g. sleep takes a time in seconds)?

Soulou commented 8 years ago

I agree that seconds sounds much more ruby-esque as you can do Time.now + 1 which means + 1 sec by default.

No problem to edit the README, sorry for the delay between the PR and my reaction :-)

Soulou commented 8 years ago

We could use sidekiq model and using a float (which is multiplied by 1000 before sending the command)

As a result, both would work:

deferred_write(1, ...)
deferred_write(10.124, ...)
bschwartz commented 8 years ago

Love the float idea. Good thinking!

On Wednesday, June 22, 2016, Soulou notifications@github.com wrote:

We could use sidekiq model and using a float (which is multiplied by 1000 before sending the command)

As a result, both would work:

deferred_write(1, ...) deferred_write(10.124, ...)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/23#issuecomment-227719841, or mute the thread https://github.com/notifications/unsubscribe/AAAOw3zZMApwSoIbbRfJ-FSoNCde0Qagks5qOSDfgaJpZM4Ix2z3 .

Soulou commented 8 years ago

Ok I'll handle it when I've a few minutes :)

Soulou commented 8 years ago

Done !

bschwartz commented 8 years ago

Thanks @Soulou. Looks good to me! Going to merge this, bump the version, and publish to ruby gems. Also adding you as a contributor ;)

freerobby commented 8 years ago

Sorry I'm late to the party on this! Thanks @Soulou for contributing here, agree with both of you that seconds works nicely for the ruby client!

bschwartz commented 8 years ago

Bumped to 1.7.0 and released on Rubygems. Thanks again for this @Soulou!