kshnurov / mandrill_dm

A basic Mandrill delivery method for Rails.
MIT License
46 stars 45 forks source link

Ability to send asynchronous messages #7

Closed beorc closed 8 years ago

kshnurov commented 9 years ago

@beorc, thank you! I've made async false by default, so it will return useful response https://github.com/jlberglund/mandrill_dm/commit/2993092e1168d0109912f65a5b2e56ac1bf37246

spovich commented 9 years ago

@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 commented 9 years ago

@kshnurov @spovich Is this fix acceptable?

spovich commented 9 years ago

@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 commented 9 years ago

@spovich Done.

spovich commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

+1 for both per app and per mailer configuration.

spovich commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Hi! I'll take care of it.

beorc commented 8 years ago

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