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

Batch Deliveries #89

Closed
excid3 opened this issue Feb 5, 2021 · 13 comments · Fixed by #313
Closed

Batch Deliveries #89

excid3 opened this issue Feb 5, 2021 · 13 comments · Fixed by #313
Labels
enhancement New feature or request

Comments

@excid3
Copy link
Owner

excid3 commented Feb 5, 2021

From @SirRawlins

Batching is something I'd love to see.

If we have a notification to deliver to several thousand recipients, then some mail services such a Postmark/Sendgrid/Mailgun allow you to batch send 1,000 messages at a time, converting 000s of API requests into a single call.

Being able to defineDeliveryMethod classes that were instantiated and enqueued with a collection of recipients, instead of just a single recipient would be nice.

Something like this pseudocode is what I imagine:

class DeliveryMethods::Postmark < Noticed::DeliveryMethods::Base

    # Define batch size on the delivery method class.
    deliver_in_batches_of 1_000

    def deliver
        post "https://api.postmarkapp.com/email/send", {
            # Has access to 1_000 recipients, instead of single recipient.
            to: recipients.map(&:email),
            subject: notification.subject
        }
    end

end

class GroupNotification < Noticed::Base

    deliveryby :database
    deliver_by :twilio
    deliver_by :postmark

end

User.all.count
# => 5000

GroupNotification.deliver_later(User.all)
# => Enqueues DeliveryMethods::Database.deliver 5,000 times.
# => Enqueues DeliveryMethods::Twilio.deliver 5,000 times.
# => Enqueues DeliveryMethods::Postmark.deliver 5 times.

This would require some refactoring of the delivery, deliver_later and run_delivery methods in Noticed::Base to batch and loop recipients if the DeliveryMethod supported it.

Not sure if the gains in efficiency and scalability would make it worthwhile though?

Originally posted by @SirRawlins in #83 (comment)

@excid3 excid3 added the enhancement New feature or request label Feb 5, 2021
@excid3
Copy link
Owner Author

excid3 commented Feb 5, 2021

I think this would make more sense to be a bit more like:

delivery_by :postmark # Sends individual emails
delivery_by :postmark, bulk: true # Sends them in bulk
class DeliveryMethods::Postmark
  def deliver 
  end

  def bulk_deliver
    recipients.each_slice(1000) do |group|
      # send group
    end
  end
end

That way you could still use Postmark to delivery individually or in bulk.

The question I have is...how does Postmark know when to actually send in bulk? It'd need to queue up the recipients somehow know when it was ready to run.

@SirRawlins
Copy link
Contributor

@excid3 yeah that's nice, I like the idea of being able to support both single and batch delivery in a single delivery method. 👍

One consideration is resiliency to failure. By putting the each_slice inside the bulk_deliver method, if one of the API calls was to fail, due to network issues or something, it would break the subsequent slices of the batch, and also make it very difficult to retry.

If the each_slice was to happen before enqueueing, then each slice would be independent of one another and could be caught with ActiveJobs retry_on in the base job class and retried.

As for your question, Postmark, Sendgrid, Mailgun and just about every other mail provider I've come across deal with bulk sending via a single POST request, with up to 1,000 email addresses in the payload, they'll then split that list, queue and deliver everything once it's on their server.

Does that make sense?

@excid3
Copy link
Owner Author

excid3 commented Feb 5, 2021

If the each_slice was to happen before enqueueing, then each slice would be independent of one another and could be caught with ActiveJobs retry_on in the base job class and retried.

Agreed. 👍

Maybe the option becomes something like this where you can specific the size of each slice.

delivery_by :postmark, bulk: { group_size: 1000 }

Then this would queue up X number of Postmark bulk delivery job (3 jobs if it had 3000 recipients).

In theory, this wouldn't change too much of the existing functionality, just add to it.

@matteomelotti
Copy link

@excid3 how to improve the bulk insert with the database option using Rails 5.2?
Trying to send 11K notifications without slow down the app

@manojmj92
Copy link
Contributor

@SirRawlins I added an implementation in #127 🙇. Let me know your thoughts!

@SirRawlins
Copy link
Contributor

@matteomelotti I saw this same issue when sending to a large number of recipients, the deliver_later method runs very slow while it adds all the jobs to the queue.

I pulled the call to deliver_later into its own background task, so it doesn't slow down the request for the user.

@SirRawlins
Copy link
Contributor

@manojmj92 awesome, I'm sorry for the slow reply, I've not been working with Noticed over the past few weeks. Your PR looks good on the face of things but will leave it for @excid3 to review properly when he gets time.

@excid3
Copy link
Owner Author

excid3 commented Jul 13, 2021

I'm leaving for a couple weeks on vacation, but I'll try and check this out when I get back.

@SirRawlins
Copy link
Contributor

@excid3 great. Have an awesome vacation, it's well deserved.

@mishaelajay
Copy link

@excid3 Any updates on the review? 😄 Would love to use noticed in a project of mine, #127 will really help.

@francesco-loreti
Copy link

Any update?

@sergioisidoro
Copy link

I would love to see this, as I'm implements Expo notifications, which supports batch delivery.
Just one caveat, that I haven't seen considered - it might be useful to consider batching by i18n language, so you can render the notification by user language. Possibly something to add to a future documentation of #127 ?

@madhums
Copy link

madhums commented Jun 20, 2023

@excid3 Could you review this? Seems like an essential feature!

@madhums madhums mentioned this issue Jan 5, 2024
Merged
9 tasks
@excid3 excid3 linked a pull request Jan 15, 2024 that will close this issue
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants