-
Notifications
You must be signed in to change notification settings - Fork 9
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
Change work_histories migration to insert via SQL #9853
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CatalinVoineag
temporarily deployed
to
review_aks
September 20, 2024 10:57 — with
GitHub Actions
Inactive
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 20, 2024 11:01
04e687e
to
13bad97
Compare
CatalinVoineag
temporarily deployed
to
review_aks
September 20, 2024 11:10 — with
GitHub Actions
Inactive
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 20, 2024 14:16
13bad97
to
f70060d
Compare
CatalinVoineag
temporarily deployed
to
review_aks
September 20, 2024 14:16 — with
GitHub Actions
Inactive
You have one or more flakey tests on this branch! ❄️ ❄️ ❄️Failed 1 out of 3 times at ./spec/system/provider_interface/provider_changes_conditions_on_an_offer_spec.rb:8: |
2 similar comments
You have one or more flakey tests on this branch! ❄️ ❄️ ❄️Failed 1 out of 3 times at ./spec/system/provider_interface/provider_changes_conditions_on_an_offer_spec.rb:8: |
You have one or more flakey tests on this branch! ❄️ ❄️ ❄️Failed 1 out of 3 times at ./spec/system/provider_interface/provider_changes_conditions_on_an_offer_spec.rb:8: |
CatalinVoineag
temporarily deployed
to
review_aks
September 20, 2024 15:20 — with
GitHub Actions
Inactive
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 23, 2024 08:54
f70060d
to
d8e77b0
Compare
CatalinVoineag
temporarily deployed
to
review_aks
September 23, 2024 08:54 — with
GitHub Actions
Inactive
You have one or more flakey tests on this branch! ❄️ ❄️ ❄️Failed 1 out of 3 times at ./spec/system/provider_interface/provider_views_a_teacher_degree_apprenticeship_application_spec.rb:25: |
CatalinVoineag
temporarily deployed
to
review_aks
September 25, 2024 15:54 — with
GitHub Actions
Inactive
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 25, 2024 15:55
55004b2
to
e19e72d
Compare
CatalinVoineag
temporarily deployed
to
review_aks
September 25, 2024 16:10 — with
GitHub Actions
Inactive
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 25, 2024 16:21
e19e72d
to
4604369
Compare
CatalinVoineag
changed the title
Backfill remaining work_histories for choices
Change work_histories migration to insert via SQL
Sep 25, 2024
CatalinVoineag
temporarily deployed
to
review_aks
September 25, 2024 16:28 — with
GitHub Actions
Inactive
elceebee
reviewed
Sep 26, 2024
elceebee
reviewed
Sep 26, 2024
CatalinVoineag
temporarily deployed
to
review_aks
September 26, 2024 12:36 — with
GitHub Actions
Inactive
elceebee
approved these changes
Sep 26, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
Previously, the data migration BackfillApplicationChoicesWithWorkExperiences was inserting records via ActiveRecord. This caused an incident, known as 'The Big Touch'. A lot of application choices, forms and candidates had their updated_at updated and this caused some of our APIs to be slow. We still need to run this migration in sandbox so we thought it's a good idea to change this migration to insert records via SQL avoiding any callbacks.
CatalinVoineag
force-pushed
the
cv/backfill-remaining-choices
branch
from
September 26, 2024 12:53
2e23b0b
to
174ed50
Compare
CatalinVoineag
temporarily deployed
to
review_aks
September 26, 2024 12:54 — with
GitHub Actions
Inactive
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
The data migration BackfillApplicationChoicesWithWorkExperiences was inserting records via
ActiveRecord. This caused an incident, known as
The Big Touch
. Many application choices, forms, and candidates had theirupdated_at
updated and this caused some of our APIs to be slow.We still need to run this migration in sandbox so we thought it's a good idea to change this migration to insert records via SQL avoiding any callbacks.
This doesn't need to run again in production, only sandbox, which has around 38k records that need updating
Ran this locally with 120k records, the workers finished their 5k batch insert in around 20 seconds, much faster than before when we were using ActiveRecord.
Changes proposed in this pull request
Changed
MigrateApplicationChoicesWorker
Guidance to review
Things to check