-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add a data migration to copy all course index data into MySQL (Take 2) #29293
Add a data migration to copy all course index data into MySQL (Take 2) #29293
Conversation
Thanks for the pull request, @bradenmacdonald! I've created OSPR-6215 to keep track of it in JIRA. As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
Your PR has finished running tests. There were no failures. |
@bradenmacdonald Thank you for your contribution. Is this ready for our review? |
@natabene Yes, thanks. |
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.
@jristau1984, @connorhaugh, @kenclary: After this goes through the pipeline, please check the log for instances of the error message this migration puts out if it finds weird data inconsistencies. It's possible that the issues we ran into before were some artifact of how we deploy and rollback database changes in the stage environment. But it could also point to some deeper logic bug or weird data race condition.
@natabene, @bradenmacdonald: FYI that the TNL team is aiming to merge/deploy this on Monday. |
@ormsbee @bradenmacdonald merging this now, will report back on any errors and have a revert PR at the ready. |
@jristau1984: thought you might like to know that bradenmacdonald merged this pull request. |
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@ormsbee @bradenmacdonald Reverted, but copying all stage logs here, so no worries of PII. Nov 22 16:50:23 ip-10-3-11-178 [service_variant=cms][common.djangoapps.split_modulestore_django.migrations.0002_data_migration][env:stage-edx-edxapp] ERROR [ip-10-3-11-178 1642] [user None] [ip None] [0002_data_migration.py:36] - Possible data issue found during data migration of course indexes from MongoDB to MySQL: Course course-v1:MITProfessionalX+Zero+Zero already exists in MySQL but the MongoDB version is newer. That's unexpected because since the course index table was added to MySQL, there has never been a time when we would write course_indexes updates only to MongoDB without also writing to MySQL. Running migrations:
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Thanks for handling this @connorhaugh. I will review that log and see if I can figure out what's causing it. Perhaps I didn't account for courses that were never published at all, or something like that? |
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
EdX Release Notice: This PR has been deployed to the production environment. |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
EdX Release Notice: This PR has been deployed to the production environment. |
@connorhaugh @ormsbee OK I see now that the course in the error log is one of those courses where there are actually two courses on stage, whose course IDs differ only by case. I suspect that's the root cause of the slightly inconsistent data. I will submit a new PR that doesn't also fail when there is no |
😞 I wonder how many collective Open edX developer weeks have been lost to case sensitivity issues around course keys. Thank you for keeping at this @bradenmacdonald. |
Description
This PR is a repeat of https://github.com/edx/edx-platform/pull/29144 which had to be reverted.
This version has similar code but when encountering unexpected data, it will just log it and proceed rather than throwing an error. See this discussion.
If the error occurs again, the log output will look like this:
Supporting information
See previous PRs.
Testing instructions
Deadline
None