Skip to content

Conversation

@awais786
Copy link
Contributor

@awais786 awais786 commented Apr 9, 2025

issue #338

Django 5.2 support.

  1. Updated the index_together syntax.
  2. Generate new migration but it changes the name of index. ( which is different from prod and other deployed instances)
  3. Upgrade the packages it was long due.

Raw sql generated by 0003 migration

python manage.py sqlmigrate celery_utils 0003

On sqlite

BEGIN;
--
-- Rename unnamed index for ('task_name', 'exc') on failedtask to idx_failedtask_task_exc
--
DROP INDEX "celery_utils_failedtask_task_name_exc_efb8c9be_idx";
CREATE INDEX "idx_failedtask_task_exc" ON "celery_utils_failedtask" ("task_name", "exc");
COMMIT;

mysql8 devstack

--
-- Rename unnamed index for ('task_name', 'exc') on failedtask to idx_failedtask_task_exc
--
ALTER TABLE `celery_utils_failedtask` RENAME INDEX `celery_utils_failedtask_task_name_exc_efb8c9be_idx` TO `idx_failedtask_task_exc`;

NOTE:
we need to find a way how to deploy this on prod, also each instance will have different index name.


PROD count. (performance impact will be decided as per records, Arbi bom will provide this)

select count(*) from celery_utils_failedtask;
+----------+
| count(*) |
+----------+
???

Testing instruction:

Set up a sandbox environment using the current master branch and populate it with sample data.
Then, upgrade to the new version of edx-celeryutils and run the migration to:

  • Ensure the migration applies cleanly
  • Verify that the new index is created as expected
  • Confirm no issues arise during the index drop/recreate process

This will help validate the change before delpoying it to staging or production.

@awais786 awais786 force-pushed the django-52-support branch from 7b504d6 to bbc8d2c Compare April 10, 2025 10:07
Copy link
Member

@UsamaSadiq UsamaSadiq left a comment

Choose a reason for hiding this comment

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

We've tested the migration effect on sandbox and everything looks good. We can merge this PR but bumping the package in edx-platform will need to wait till SRE team has confirmed the migration change effect on db.

@feanil
Copy link
Contributor

feanil commented Apr 22, 2025

@UsamaSadiq is that work schedule by the SRE team at 2U? I don't want wait for too long for that to be completed.

new_name='idx_failedtask_task_exc',
old_fields=('task_name', 'exc'),
),
]

Choose a reason for hiding this comment

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

ALTER INDEX "celery_utils_failedtask_task_name_exc_efb8c9be_idx" RENAME TO "idx_failedtask_task_exc";

@awais786 here is the query it is generating for postgresql.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, this is also a low cost metadata only query so it should be safe to run on even large tables.

Choose a reason for hiding this comment

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

agreed!

@feanil
Copy link
Contributor

feanil commented Apr 23, 2025

@awais786 can you update the tox and github workflows to also run tests on django 5.2?

@UsamaSadiq
Copy link
Member

@UsamaSadiq is that work schedule by the SRE team at 2U? I don't want wait for too long for that to be completed.

Yes, I've verified this change on sandbox and have created an issue for the SRE team to verify on large tables. We should be hearing back from them by the day end tomorrow and then we can proceed with merging this PR along with others.

@UsamaSadiq
Copy link
Member

UsamaSadiq commented Apr 24, 2025

Tested the potential effect of the index_together --> indexes change on prod db replica. We've verified that it won't be a blocker for us on the large tables as well. Further details are mentioned in the discovery issue specifically in the latest comment.

arbi-bom team has completed our testing and will now move ahead with merging the PRs.

FYI @feanil

@UsamaSadiq UsamaSadiq merged commit 8455633 into openedx:master May 6, 2025
12 checks passed
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.

4 participants