Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support DPUB to send deferred messages #23

Merged
merged 3 commits into from
Jun 28, 2016
Merged

Conversation

Soulou
Copy link
Contributor

@Soulou Soulou commented Jun 9, 2016

No description provided.

@ghost
Copy link

ghost commented Jun 9, 2016

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

@bschwartz
Copy link
Member

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

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

Soulou commented Jun 22, 2016

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

Soulou commented Jun 22, 2016

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

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
#23 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAOw3zZMApwSoIbbRfJ-FSoNCde0Qagks5qOSDfgaJpZM4Ix2z3
.

@Soulou
Copy link
Contributor Author

Soulou commented Jun 27, 2016

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

@Soulou
Copy link
Contributor Author

Soulou commented Jun 27, 2016

Done !

@bschwartz
Copy link
Member

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

@bschwartz bschwartz merged commit 25aa0cb into wistia:master Jun 28, 2016
@freerobby
Copy link
Member

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants