search

TabbycatDebate / tabbycat

Debating tournament tabulation software for British Parliamentary and a variety of two-team parliamentary formats
https://tabbycat.readthedocs.io/
GNU Affero General Public License v3.0
245 stars 827 forks source link

Email notifications: sent message records not stored on timeout #847

Closed czlee closed 6 years ago

czlee commented 6 years ago

The symptom I noticed was that when resending private URL emails after generating new ones, it sent it to the entire tournament again, rather than just those who hadn't already received emails. (I checked this in my SendGrid logs, and also got two emails myself.) This issue is mainly just to keep track of it, I still need to investigate its reproducibility before confirming as a bug.

czlee commented 6 years ago

Actually, the cause of this is that sent message records aren't being stored, which I think is in turn due to a H12 timeout. So it's not a filter thing.

tienne-B commented 6 years ago

Also the filter is not working as it should because we can't use OuterKey() in a JSON field. Is H12 only on Heroku? I've not had this problem locally.

I guess this is another reason for #740... also check the performance of the TournamentEmailMessage

czlee commented 6 years ago

Yeah, H12 is only a Heroku thing, and you'll probably only see it when sending emails to a couple of hundred people. So it seems like a scaling issue. It's weird though, we've dealt with bigger tournaments before, maybe SendGrid changed how it works or something.

tienne-B commented 6 years ago

We have changed the generation here though, which may be the cause of the performance hit.

czlee commented 6 years ago

Since we're emailing judges who didn't get their links manually, I should hopefully have some sense of the scale of the problem some time during round 1. Hopefully. I don't know how much log parsing this will involve.

tienne-B commented 6 years ago

What I think caused the performance hit is that now the preferences are looked up in the TournamentMessage class, and not passed to it, which may create a nx(N+1) database problem (reply-to, tournament name, subject template, message template). These values could just be passed by the generators, like it used to. I've discussed this previously with @philipbelesky. I'll compare performance on both.

czlee commented 6 years ago

I realized I was running on a sole free dyno, and finally pushed it up to performance dynos to try the emails again. It worked like a charm. So most of this was just me being stupid.

That said, we probably want more of a safety net in here. I have more thoughts on this, I'll post them after the tournament.

philipbelesky commented 6 years ago

I'll wait for your additional thoughts, but the timeout issue at least should theoretically be solved by #849 now.

czlee commented 6 years ago

After reflecting on this a couple of weeks, the safety net is basically #875, because that allows someone to use their own knowledge to narrow down emails, or at least send in batches. Also, #849 definitely helps, and if we ever do #848 then that'll help too.