missed-emails-sending: Move email sending to separate…

[kkanahin]

… queue.

• Add new 'missedmessage_email_senders' queue for sending missed messages emails.
• Add the new worker to process 'missedmessage_email_senders' queue.
• Split aggregation missed messages and sending missed messages email
  to separate queue workers.
• Adapt tests for sending missed emails to the new logic.

Fixes #2607

[smarx]

Automated message from Dropbox CLA bot

@kkanahin, it looks like you've already signed the Dropbox CLA. Thanks!

[timabbott]

I think this change adds an extra database query; why did you make the change?

[kkanahin]

On the contrary, I've decrease queries quantity in this place. but yes it would be better to use boolean condition here for queryset.

[timabbott]

But don't we need to fetch the messages anyway later on, so we end up doing a database query for the boolean check and then a second to fetch the message IDs (if any were returned)? Whereas previously we did a single query unconditionally.

It is a cheaper query in the case that there are no remaining unread messages in the list, but that case is rare.

[kkanahin]

@timabbott I've agreed with you in my previous comment :)

[kkanahin]

you are right, we should remove exists from condition.

[timabbott]

@kkanahin thanks for working on this! I just got a chance to review this. I think this implementation should work, but it looks like this takes an approach of putting the arguments to do_send_missedmessage_events_reply_in_zulip in a queue. It seems like we could instead just put the data we pass to EmailMultiAlternatives in a queue as I had suggested in the issue description; looking at this code:

    msg = EmailMultiAlternatives(subject, text_content, from_email, [user_profile.email],            
                                 headers = headers)                                                  
    msg.attach_alternative(html_content, "text/html")                                                
    msg.send()                                                                                       
                                                                                                     
    user_profile.last_reminder = timezone.now()                                                      
    user_profile.save(update_fields=['last_reminder'])                                               

it seems like the resulting system would be somewhat cleaner, since then the email sending queue could actually be potentially reused for other things that send lots of emails as well.

What do you think about switching to the approach I suggested?

[kkanahin]

I think we shouldn't put email data to the queue, cause it can overload rabbitmq server when we will send more than 500 emails. All emails data will go through the message queue. That's why I've made such implementation.

[kkanahin]

I can switch to your implementation, it doesn't take a lot of time. And we can test performance. Should I do it @timabbott ?

[timabbott]

Yeah, maybe do the other approach in a new PR (saving this code in case we decide we want it?). I think performance won't be an issue; my guess is that the part before it starts trying to send an email is just a few ms (whereas sending email with authentication is usually a few 100ms, as part of anti-spam strategies from email providers), but you can test that in development (should be similar in production) to confirm.

[kkanahin]

sure @timabbott Will do it in another branch.

[timabbott]

I think the new version is cleaner, so closing this.