Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor version history (PART 1/2) #8041

Merged
merged 32 commits into from
Jul 16, 2024
Merged

Conversation

kurtisassad
Copy link
Contributor

@kurtisassad kurtisassad commented Jun 4, 2024

This will be the first ticket of two in an expand/contract database migration pattern. This is the expand portion where we will create the new entries for version_history, but not delete the old ones until part 2.

Link to Issue

Closes: #7812

Description of Changes

  • Adds schema changes for new version_history to be added into their own tables
  • Adds application level changes to add the new version_histories to this new table (While still keeping the original version_history functionality)
  • Adds migration script that we will run async to reach parity with the new and old verion_history schemas.

Test Plan

  • Create thread (make sure select * from "ThreadVersionHistories"; has correct data)
  • Update thread (make sure select * from "ThreadVersionHistories"; has correct data)
  • Create comment (make sure select * from "CommentVersionHistories"; has correct data)
  • Update comment (make sure select * from "CommentVersionHistories"; has correct data)

Deployment Plan

  1. Make a regular deployment
  2. Wait for database migration to finish
  3. Run ssh into heroku with heroku run bash -a [appname] then in the packages/commonwealth directory run pnpm run async-migrate-thread-version-history and then pnpm run async-migrate-comment-version-history

@kurtisassad kurtisassad added the deployment plan (PRs only) requires manual infrastructure changes on release label Jun 14, 2024
@kurtisassad kurtisassad marked this pull request as ready for review June 20, 2024 15:50
Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

2 quick comments

Copy link
Contributor

@Rotorsoft Rotorsoft left a comment

Choose a reason for hiding this comment

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

I don't see any model associations... expecting comment with many comment versions and thread with many thread versions

Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

I created a thread which successfully created an equivalent version history entry but when I edited the same thread a new version history entry was not created.

Also, we should reorder the migrations so that the version history migration comes last.

@kurtisassad
Copy link
Contributor Author

kurtisassad commented Jul 11, 2024

I don't see any model associations... expecting comment with many comment versions and thread with many thread versions

There are associations but I put them in the models. This is because I could not get them to work in the associations.ts file. We should maybe think of going back to the original model (The one described in the sequelize docs) because using our custom associations implementation gets complicated when things break. Maybe we can discuss it during the friday platform meeting.

@kurtisassad
Copy link
Contributor Author

edited the same thread a new version history entry was not created.

Good catch, fixed.

Also, we should reorder the migrations so that the version history migration comes last.

Done

@kurtisassad
Copy link
Contributor Author

Ran it on frack, works correctly for both the thread and comment migrations (But takes a while)

@timolegros
Copy link
Collaborator

Ran it on frack, works correctly for both the thread and comment migrations (But takes a while)

2 final questions:

  • Will we need to run the script multiple times if comments/threads are created after the script starts given that the count is only retrieved at the start of the script?
  • Do we need to sleep between each query given that comments are longer batched (limited to 1 per iteration) which could mean a ton of queries are generated in a short amount of time? (What was impact on Frack DB).

@kurtisassad
Copy link
Contributor Author

kurtisassad commented Jul 15, 2024

Will we need to run the script multiple times if comments/threads are created after the script starts given that the count is only retrieved at the start of the script?

It will not be necessary, since any new threads/comments would be marked with version_history_updated true. Although it wouldn't hurt to run it a 2nd time just to verify it doesn't update anything (Since it will mark all the old version histories version_history_updated as true after migration).

Do we need to sleep between each query given that comments are longer batched (limited to 1 per iteration) which could mean a ton of queries are generated in a short amount of time? (What was impact on Frack DB).

I hadn't checked. There was no noticeable slowdown on frick, but I was probably the only one using it while the script was running. I can add some sleep time, but it might be better to just run it plain. We can always abort the script if it slows down the site (And resume from where we stopped).

@rbennettcw rbennettcw self-requested a review July 16, 2024 13:15
@kurtisassad kurtisassad merged commit 62fde4c into master Jul 16, 2024
9 checks passed
@kurtisassad kurtisassad deleted the ka.versionHistoryRefactor branch July 16, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment plan (PRs only) requires manual infrastructure changes on release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix version_history DB Schema
4 participants