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

Ability to send asynchronous messages #7

Merged
merged 1 commit into from
Jan 23, 2016
Merged

Conversation

beorc
Copy link
Contributor

@beorc beorc commented Feb 13, 2015

No description provided.

@kshnurov
Copy link
Owner

@beorc, thank you! I've made async false by default, so it will return useful response 2993092

@kshnurov kshnurov closed this Feb 22, 2015
@kshnurov kshnurov reopened this Feb 23, 2015
@spovich
Copy link
Contributor

spovich commented Feb 23, 2015

@kshnurov this broke the tests. Please include updated tests in this PR. There are still other broken tests from your merges that I'm trying to fix.

@beorc
Copy link
Contributor Author

beorc commented Feb 25, 2015

@kshnurov @spovich Is this fix acceptable?

@spovich
Copy link
Contributor

spovich commented Feb 25, 2015

@beorc Looks like you need to rebase your fork/branch on latest upstream master. That commit is super noisy. It should just be adding the async stuff and updating the associated tests.

@beorc
Copy link
Contributor Author

beorc commented Feb 26, 2015

@spovich Done.

@spovich
Copy link
Contributor

spovich commented Feb 26, 2015

@beorc Thank you for fixing up the tests. I haven't had a need for async personally, but I understand that it is useful for bulk sending. It seems to me that this might be more useful and flexible if it was configured per mailer rather than as you have it configured (per application)? @jlberglund @kshnurov any thoughts?

Also, the Mandrill API has a few other method args for send that we aren't exposing that might be good to handle. The full arg list is: send(message, async=false, ip_pool=nil, send_at=nil)

@kshnurov
Copy link
Owner

It would be great to have both per app and per mailer configuration. Also we should add an ability to specify async as an argument: mail(..., :async => true)

Btw, we've encountered an unpleasant "feature" of Mandrill - messages/info is not always available right after you've sent a message (even with async = false), sometimes you may have to wait a lot (up to 30 minutes). Here's a response from Mandrill support:

When we're under heavy load, we stop running lower-priority tasks (indexing 'send' data fits into this category) for a period of time to ensure that we have the resources necessary for delivery, so at peak times, it could take a few minutes before the system has that data available. Load level can also vary from server to server, so some messages may be indexed quickly while others experience a longer delay.

What some our users have done, and what we recommend, is process email sending and delivery checking as an asynchronous process. By this we mean that you could have your application send email and store each message id and then have logic in place to check for the status of those messages 20 to 30 minutes later.

You should keep that in mind if you're using async = true by default (and it's always on when there are more than 10 recipients) - sometimes you won't be able to get any info about that message for a long time. If you want to know in a short period of time if it was at least sent or rejected, you should not use async = true. Better use sidekiq to send mail asynchronously.

@jlberglund
Copy link
Contributor

Thanks, @beorc. I've not had the need for async yet, but I've considered it. @kshnurov thanks for sharing that insight from Mandrill support. I had no idea. I agree with @spovich about sending per mailer, but I see no problem with a default per app also. Maybe something which can be unobtrusively set in a config file (e.g. through an options array).

@spovich
Copy link
Contributor

spovich commented Feb 26, 2015

+1 for both per app and per mailer configuration.

@spovich
Copy link
Contributor

spovich commented Jan 21, 2016

I think this is good to merge as it is now with the default to false. Only thing I'd like to see is the README updated with this PR to reflect the new app-level config option.

@spovich
Copy link
Contributor

spovich commented Jan 22, 2016

Hi @beorc, @jlberglund has transferred maintenance of this gem to me. I've made a few small updates to reflect those changes. Please rebase this PR and add a note to the README about async, and then it should be good to merge. Thanks!

@beorc
Copy link
Contributor Author

beorc commented Jan 22, 2016

Hi! I'll take care of it.

@beorc beorc force-pushed the master branch 6 times, most recently from 949535f to 5dbff52 Compare January 23, 2016 08:05
@beorc
Copy link
Contributor Author

beorc commented Jan 23, 2016

@spovich I've rebased PR and added a section to the README about configuration options.

spovich pushed a commit that referenced this pull request Jan 23, 2016
Ability to send asynchronous messages
@spovich spovich merged commit 9594fe1 into kshnurov:master Jan 23, 2016
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.

4 participants