Skip to content

Conversation

@mtyaka
Copy link
Contributor

@mtyaka mtyaka commented Nov 17, 2016

The new version extends the course_id field to support courses with course keys longer than 50 characters: open-craft/problem-builder#130

Note that this includes changes from https://github.com/edx/edx-platform/pull/13953, which hasn't been merged yet.

Below is the output of sqlmigrate for the new migration:

BEGIN;
ALTER TABLE `problem_builder_answer` MODIFY `course_id` varchar(255) NOT NULL;

COMMIT;

Environments: edx.org and edge.edx.org

Merge deadline: ASAP - this is blocking a course on Edge

JIRA tickets: TNL-5932

Discussions: See JIRA ticket.

Dependencies: https://github.com/edx/edx-platform/pull/13953

Sandbox URL:

Partner information: hosted on edX Edge (Davidson College)

Testing instructions:

Check that you can successfully add a Problem Builder "Long Answer" component to a course with a course key that is longer than 50 character. See working example on the sandbox: https://studio-pr14013.sandbox.opencraft.hosting/container/block-v1:VeryLongOrganizationName+VeryLongCourseName+VeryLongCourseRun2016+type@vertical+block@44e0d2e57f3840faa7278164960338ff

Compare this to the broken problem on the old (v2.6.2) sandbox where I created a course with the same course key length and tried to add a "Long Answer" component to it: https://studio-pr13953.sandbox.opencraft.hosting/container/block-v1:VeryLongOrganizationName+VeryLongCourseName+VeryLongCourseRun2016+type@vertical+block@0ce09c73c6874411907ff24cbd325394

Author notes and concerns:

  1. problem-builder v2.6.3 includes changes from https://github.com/edx/edx-platform/pull/13953, which has not been merged into edx-platform yet.
  2. This PR contains a migration of a potentially large MySQL table, we would like someone from @edx/devops to review it.

Reviewers

Settings

NGINX_SET_X_FORWARDED_HEADERS: false
EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/open-craft/problem-builder.git@v2.6.3#egg=xblock-problem-builder==2.6.3

@pomegranited
Copy link
Contributor

FYI @gsong This PR affects the edx.org and edge.edx.org environments.

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 Thanks for doing this change, @mtyaka !

  • I tested this on your sandbox.
  • I reviewed the code change, paying attention to the installed version tag, and verifying that it matches the tag on problem-builder.
  • Accessibility No GUI changes; a11y not affected
  • Includes documentation Bugfix, no documentation required.

@jibsheet
Copy link
Contributor

@maxrothman As the oncall, can you work with OpenCraft on this. Prod has a 4 million row table being migrated, but depending on the ALTER it may be ok. Edge is much smaller.

@mtyaka Please provide the output of sqlmigrate for this migration.

@gsong
Copy link
Contributor

gsong commented Nov 17, 2016

@mtyaka or @pomegranited Can you put the deployment targets at the top of the PR description? Thank you!

@mtyaka
Copy link
Contributor Author

mtyaka commented Nov 17, 2016

@jibsheet @maxrothman This is the output of sqlimigrate for the migration included in this version bump:

$ ./manage.py lms --settings=devstack sqlmigrate problem_builder 0003_auto_20161116_0730
BEGIN;
ALTER TABLE `problem_builder_answer` MODIFY `course_id` varchar(255) NOT NULL;

COMMIT;

It changes the course_id columns from varchar(50) to varchar(255).

Copy link
Contributor

@adampalay adampalay left a comment

Choose a reason for hiding this comment

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

👍 from a code POV, but would like devops's insight into how the migration might affect us

@openedx-webhooks
Copy link

Thanks for the pull request, @mtyaka! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

@maxrothman
Copy link
Contributor

This migration takes 8 minutes to run on our production database. With ~10+ updates to the table per hour, we'd be likely to cause user-facing issues. Instead, I'd recommend we do this in a few steps:

  1. create a migration that adds a new column and alter the code to write to the new column and read from the new column, falling back on the original column
  2. create a data migration that copies values from the old to the new column. edX will fake this migration and provide a script for doing this migration in batches.
  3. create a migration that removes the old column and make the code only use the new column

Steps 1 and 3 will have to take place in separate releases, so at minimum we'll have to spread this over 2 weeks.

@mtyaka
Copy link
Contributor Author

mtyaka commented Nov 21, 2016

Thanks @maxrothman, in that case we will prepare multiple migrations and problem-builder releases that do this in steps, as per your suggestion.

mtyaka referenced this pull request in open-craft/problem-builder Nov 24, 2016
Migration locks the table, which is not desired since the production
table on edx.org is quite large.

The course_id field will be extended in a multiple steps instead.
For more info see:

- https://github.com/edx/edx-platform/pull/14013
- #131
@mtyaka
Copy link
Contributor Author

mtyaka commented Nov 25, 2016

Closing this in favor of https://github.com/edx/edx-platform/pull/14044

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants