Ensure all push actions are deleted from staging #2894
Conversation
| @@ -1162,16 +1162,17 @@ def _update_metadata_tables_txn(self, txn, events_and_contexts, backfilled): | |||
| backfilled (bool): True if the events were backfilled | |||
| """ | |||
|
|
|||
| # Insert all the push actions into the event_push_actions table. | |||
There was a problem hiding this comment.
stick this after lines 1171-1173?
There was a problem hiding this comment.
We want to ensure that it always gets run, so that it clears up the staging area with all events.
There was a problem hiding this comment.
right, but if events_and_contexts is falsy, then this is a no-op, so you might as well move it down?
There was a problem hiding this comment.
Its not a noop if all_events_and_contexts isn't empty.
There was a problem hiding this comment.
right, but it is if it is, which is what we're talking about here.
I feel like we must be at cross-purposes here, so let me clarify. I am suggesting changing:
self._set_push_actions_for_event_and_users_txn(...)
if not events_and_contexts:
return
to:
if not events_and_contexts:
return
self._set_push_actions_for_event_and_users_txn(...)
(which, incidentally, also resolves the discussion below)
There was a problem hiding this comment.
oh ffs I was reviewing a previous commit and not realising that all_events_and_contexts was added later.
There was a problem hiding this comment.
Ah, oops. Yay go github
| }, | ||
| retcol="user_id", | ||
| ) | ||
| if events_and_contexts: |
There was a problem hiding this comment.
I'm not 100% convinced that we events_and_contexts can't be empty, given the number of times we filter it down. The rest of the code should be fine with it being empty.
There was a problem hiding this comment.
and why is this code not ok with it being empty?
There was a problem hiding this comment.
Maybe it is? But there isn't any documentation that executemany is fine with empty lists and its the sort of thing I can imagine it complaining about.
| tuples: list of tuples of (user_id, actions) | ||
| events_and_contexts (list[(EventBase, EventContext)]): events | ||
| we are persisting | ||
| all_events_and_contexts (list[(EventBase, EventContext)]): all |
There was a problem hiding this comment.
it's a lovely parameter, but it doesn't seem to do much.
…ndle_unpersisted_events_push
Sometimes we get events multiple times over federation, and we end up calculating and inserting the push rules for these events into the staging area. This PR ensures that all events that were going to be persisted have their staging push actions cleared.
Note: This includes a refactor as the first commit.