Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

delete_local_events for purge_room_history #2858

Merged
merged 5 commits into from
Feb 9, 2018
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Feb 8, 2018

Add a flag which makes the purger delete local events

@richvdh
Copy link
Member Author

richvdh commented Feb 9, 2018

retest this please

):
"""Deletes room history before a certain point


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this too much whitespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

@ara4n
Copy link
Member

ara4n commented Feb 9, 2018

lgtm other than stupid whitespace query.

I assume the new 'allow_empty_body' thing is necessary because previously we just crashed on trying to parse '' as JSON.

finally: i always feel a bit scared of the purge API; when we first implemented it there were dire warnings about "this will lock your synapse solid" or "you must restart once you've purged" etc. If this is no longer true, we might want to call it out in the doc.

@richvdh
Copy link
Member Author

richvdh commented Feb 9, 2018

I assume the new 'allow_empty_body' thing is necessary because previously we just crashed on trying to parse '' as JSON.

we'd return a 400 NOT_JSON error or something. Sending an empty body seems like a thing that people would have been likely to do, so I'm trying to keep that working.

finally: i always feel a bit scared of the purge API; when we first implemented it there were dire warnings about "this will lock your synapse solid" or "you must restart once you've purged" etc. If this is no longer true, we might want to call it out in the doc.

It is, evidently, still true. There's an argument that should be called out in the doc...

@@ -2330,6 +2324,16 @@ def _purge_history_txn(
]
)

# the event peristence loop takes out an exclusive lock on room_depth
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i stared at this stupidly thinking that "event persistence loop" meant the loop immediately above which is purging and persisting the new events... rather than the general BAU activity of persisting events.

If you can clarify that you're talking about "synapse takes out an exclusive lock on room_depth whenever it persists events..." it would avoid confusing bears of very little brain such as I

@ara4n
Copy link
Member

ara4n commented Feb 9, 2018

re-lgtm other than the comment-which-i-found-confusing

(beacause it deletes more than state)
this thing takes ages and the only sign of any progress is the logs, so having
some logs is useful.
it was a bit of a non-sequitur there
Add a flag which makes the purger delete local events
... to avoid locking the table for too long
@richvdh richvdh merged commit 10b34db into develop Feb 9, 2018
@richvdh richvdh deleted the rav/purge_updates branch May 2, 2018 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants