worker: Make MissedMessageWorker not be lossy. #19119
Conversation
|
@mateuszmandera Would be great if you could take a look! |
|
@abhijeetbodas2001 thanks for working on this! I'm really excited about it, and posted a batch of comments. One thought I have is that it'd be nice to have a test suite for this logic; I wonder if it makes sense to as part of setting that up, move the non-timer parts of the system into another file that we write a test suite for? Not sure if that's worth it. |
abhijeetbodas2001
force-pushed
the
grace
branch
2 times, most recently
from
ccdbcb9
to
44975c8
Compare
|
@timabbott This is ready for another review. I posted the changes in inline comments.
I've expanded the test to verify that the rows in the new table are formed correctly, and that the time calculation works correctly. |
|
The test looks great! I started https://chat.zulip.org/#narrow/stream/101-design/topic/missed.20message.20email.20delay/near/1226912 to confirm the semantics of what we want here. |
abhijeetbodas2001
force-pushed
the
grace
branch
3 times, most recently
from
6f19600
to
cd02d99
Compare
|
@andersk @timabbott This is ready for another review, but please see #19170 first! |
zerver/models.py
Outdated
| user_profile: UserProfile = models.ForeignKey(UserProfile, on_delete=CASCADE) | ||
|
|
||
| message: Message = models.ForeignKey(Message, on_delete=CASCADE) | ||
| trigger: str = models.TextField(max_length=20) |
There was a problem hiding this comment.
Do we need a max_length on this? That's only required for CharField, and I don't think there's a reason to have one for this.
There was a problem hiding this comment.
I changed this to use choices. There's only 4 string values this field can take.
zerver/models.py
Outdated
| @@ -3313,6 +3313,23 @@ def increment_times_used(self) -> None: | |||
| self.save(update_fields=["times_used"]) | |||
|
|
|||
|
|
|||
| class MissedMessageEmailEntry(models.Model): | |||
There was a problem hiding this comment.
What do you think about calling this ScheduledNotificationEmailTrigger? I think that might convey the intention a bit more clearly, with analogy to ScheduledEmail and ScheduledMessage.
Or maybe even ScheduledMessageNotificationEmail? One would read that as this being a "message notification" email that we've scheduled for the target time, and that we additionally have an algorithm for collapsing several of these when we get to actually sending emails.
There was a problem hiding this comment.
(I think there's a good chance that we're going to rename MissedMessage across the codebase, since we changed the user-facing name for the email sender from "Zulip missed messages" to "Zulip [email is implicit] notifications", and we plan to have configurations that send these even if you see the message)
There was a problem hiding this comment.
ScheduledMessageNotificationEmail sounds good, updated to use that 👍
zerver/models.py
Outdated
| ) | ||
|
|
||
| # Calculated from the time event was received and the batching period. | ||
| expiry_time: datetime.datetime = models.DateTimeField(db_index=True) |
There was a problem hiding this comment.
Maybe scheduled_timestamp would be appropriate? I think it has very similar semantics to the fields of that name in ScheduledEmail and ScheduledMessage.
abhijeetbodas2001
force-pushed
the
grace
branch
2 times, most recently
from
7bec0d6
to
4f884a6
Compare
|
@timabbott Updated the PR with the suggested changes and #19119 (comment), along with an error I had made previously regarding the This is ready for another review. |
Previously, we stored up to 2 minutes worth of email events in memory before processing them. So, if the server were to go down we would lose those events. To fix this, we store the events in the database. This is a prep change for allowing users to set custom grace period for email notifications, since the bug noted above will aggravate with longer grace periods.
|
I merged the first couple commits as the series ending with 8ea15e70eff1eadec027027c0746eb88f3bdf53d, after some tweaks to the comment for the new model. And then shortly after, I merged the last commit as well. @abhijeetbodas2001 this should unblock the follow-up duration setting PR. @alexmv FYI -- we'll want to pay attention to this PR when doing production deployments. I'm not sure the model of having all messages for an individual user have the same timestamp at which they are processed is correct; it seems possible we instead want that behavior to be per-topic; I haven't really thought it through. But critically, what we've implemented here is I believe identical to what we had before merging this, so there's no regression in any case. That's probably worth discussing in #design a bit; want to start a thread? |
Previously, we stored up to 2 minutes worth of email events in memory
before processing them. So, if the server were to go down we would lose
those events.
To fix this, we store the events in the database.
This is a prep change for allowing users to set custom grace period for
email notifications, since the bug noted above could aggravated if with
longer grace periods.
Testing plan:
GIFs or screenshots: