emencia / emencia-django-newsletter

An app for sending newsletter by email to a contact list.
189 stars 72 forks source link

Per hour count of messages on smtp could be improved #36

Closed alexgarel closed 12 years ago

alexgarel commented 13 years ago

EDN has a per hour limit on smtp which is very useful

In my scenario I had two mailing lists to send where the client is very picky on mail schedule over time because i falls on its equipment.

One was to be send at a rate of 4400 / h and the other at 2200 / h all day long, so I set up each newsletter on it's own smtp.

I set my send_newsletter cron 4 times an hour.

so far, so good.

The problem is that I had a shift over time, putting me behind schedule.

In fact first wave is ok. But first mail is sent at 9:00 whereas last is sint at 9:15 So next time at 10:00 only few mails get sent as those sent at 9:01 are taken into account in last hour charge, so we catch up with 10:15 cron task but last mail is sent at maybe 10:30. After some time we are quite far from schedule !

I don't really see how to solve this problem right now but I may think about it :-)

sergiodlc commented 13 years ago

I'm new to EDN and I hope to send my contributions in the next few days. I have already noticed that the throttling mechanism already implemented should be improved. Some SMTP servers can have a burst limit (minimum time between emails) so there should be the possibility of evenly distribute the sending requests along the time span when the number of emails to be sent is greater than the rate limit. I think this should help with your problem. It would be interesting to see how does this feature interact with several cron jobs that overlaps over time.

alexgarel commented 13 years ago

My current thoughts on this problem, even if I don't have time for this right now is to have a daemon instead of a cron The daemon would compute next time to wakeup to send mail (and waking up at least each half hours to check new sending process). So emailing could be continuous which would solve the problem.

danielsokolowski commented 12 years ago

I have just hit this 'minimum time between emails' burst limit with google smtp servers. In my mind if one specifies 60 emails an hour it implicitly assumes one email per minute not, 60 emails as fast as you can then stop.

I have implemented this change in my branch, before a pull request though I welcome a discussion if one ought provide both modes, space out sending evenly or do as fast as you can; I am in the camp space out sending is the only mode that's needed.

Direct commit link: https://github.com/danols/emencia-django-newsletter/commit/8a648c1bfd337707506bc23f6b0c346c9c936132 (https://github.com/danols/emencia-django-newsletter)

alexgarel commented 12 years ago

Yes I also think "space out sending evenly" is a saner approach and the only one needed.

Regarding your patch I think this won't work well : your process may finish at 1h+1s, so if you planned a cron every hour, the next will start 59mn59s later without mails sent in between. So I think you have to change the code so after processing your current "expedition list" you compute the new one and continue sending. I think get_expedition_list has to be computed in another manner.

I may look at it on monday if you want.

danielsokolowski commented 12 years ago

Please feel free to go right ahead, because I believe the '59m59s' second issue I just experienced.

On 14/01/2012 12:56 PM, Alex Garel wrote:

Yes I also think "space out sending evenly" is a saner approach and the only one needed.

Regarding your patch I think this won't work well : your process may finish at 1h+1s, so if you planned a cron every our, the next will start 59mn59s later without mails sent in between. So I think you have to change the code so after processing your current "expedition list" you compute the new one and continue sending. I think get_expedition_list has to be computed in another manner.

I may look at it on monday if you want.


Reply to this email directly or view it on GitHub: https://github.com/Fantomas42/emencia-django-newsletter/issues/36#issuecomment-3493818

danielsokolowski commented 12 years ago

Now that I think about it this shouldn't be an issue, the code generates next expedition list based on those that have not yet been sent, and each sent status gets updated on every send loop; hence next cron job will generate the next expedition list from those that have not yet been sent. New updated code that exits after 3 critical failures rather then hammering the server for the entire hour can be found here --- https://github.com/danols/emencia-django-newsletter/commit/82ce8ce5f2bcfc2e52a8f9f69eda2fcbbb68890e

alexgarel commented 12 years ago

Here is my proposal https://github.com/Fantomas42/emencia-django-newsletter/pull/53

I completely reworked the way the mails are sent, driving by the smtp server, with a round-robin repartition between NL, sending equitably along the time to respect smtp server limit.

use it with bin/ send_newsletter_continuous

bin/ send_newsletter is the old implementation.

@danols I think your update concerning not hammering the server in case of error shall be in another issue, this is not the same matter. Note that in my continuous mode script, I write such error on stderr so in a prod environment you could monitor this issues.

danielsokolowski commented 12 years ago

Your using words I recall from school so it ought to be a good implementation :) - I'm taking a look now, on a side note what do you think of using python logging facilities rather then stderr ?

alexgarel commented 12 years ago

we may use logging of course, but I didn't have time right now + I am not a Django specialist so I do not know how it handles that + for a script (which is not an application) a straight print to stderr is not so bad.

The idea is that I just put real production errors on stderr. You would redirect stderr to a log file so you woud be able to monit for it in a production environment (eg. Monit can see if it grows regularly and send an alert to administrators).

using logging is better though, you're right, feel free to improve, but do this in a separate issue :-)

Fantomas42 commented 12 years ago

The pull request #56 has been validated and applied, so I close this issue.

Thank to alexgarel and danols for their participations.