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

We have instructions telling people to run the bg job 'populate_stats_process_rooms', but it is a no-op #8238

Closed
anoadragon453 opened this issue Sep 3, 2020 · 5 comments · Fixed by #8243
Assignees
Labels
z-regression (Deprecated Label)

Comments

@anoadragon453
Copy link
Member

Description

In this delta file we converted populate_stats_process_rooms to a no-op and created populate_stats_process_rooms_2 in order to get around some edge cases with older Synapse versions.

It seems that this was not necessary, and we could've kept the same name and simply purged the progress of and restarted background job in the delta file instead.

The chosen solution has problems as there are places that we tell people to use populate_stats_process_rooms to fix issues. These instructions now simply do nothing as the bg job is a no-op.

A DELETE was used to solve a similar problem in another delta file:

DELETE FROM background_updates WHERE update_name IN (

The delta file should be updated to sort out this mess.

@reivilibre
Copy link
Contributor

It seems to me that one could add a new delta and UPDATE _2 jobs to the old name (and of course modify the code to make the original name not a NOP).

(Fixing the existing delta file is not necessarily good as it has already gone out, and someone may be half-way through running the job when they upgrade to the version where we back out the _2 job)

@anoadragon453 anoadragon453 changed the title Convert populate_stats_process_rooms back from a no-op to a background job We have instructions telling people to run the bg job 'populate_stats_process_rooms', but it is a no-op Sep 3, 2020
@anoadragon453
Copy link
Member Author

@reivilibre I was planning to delete the progress again, but you're right in that I think we could keep the user's existing progress by just creating a new delta file. Thanks :)

@anoadragon453 anoadragon453 added p1 z-regression (Deprecated Label) labels Sep 3, 2020
@anoadragon453
Copy link
Member Author

Oh, but we'll need to delete the progress anyways to cover the edge cases laid out in the original problematic delta file...

@reivilibre
Copy link
Contributor

reivilibre commented Sep 3, 2020

You wouldn't need to delete the progress made by a _2 job, right?

i.e. a delta which:

  • deletes populate_stats_process_rooms
  • renames populate_stats_process_rooms_2 to populate_stats_process_rooms

should do as intended?

@anoadragon453
Copy link
Member Author

@reivilibre After running through the possible upgrade paths through the notable versions (<=v1.18.0, v1.19.0, and $next_version), this suggestion seems to work in all cases. Thank you!

anoadragon453 added a commit that referenced this issue Sep 8, 2020
…ate_stats_process_rooms' again (#8243)

Fixes #8238

Alongside the delta file, some changes were also necessary to the codebase to remove references to the now defunct `populate_stats_process_rooms_2` background job. Thankfully the latter doesn't seem to have made it into any documentation yet :)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
z-regression (Deprecated Label)
Projects
None yet
2 participants