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

Atomic transaction for the migrate_transcripts command #1067

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

OmarIthawi
Copy link

@OmarIthawi OmarIthawi commented Jan 25, 2022

A course will either be successful or will fail in one atomic SQL transaction.

Jira: RED-2483.

TODO

  • Add tests.
  • Test on staging, no exceptions were found.

@github-actions

This comment has been minimized.

@coveralls
Copy link

coveralls commented Jan 25, 2022

Coverage Status

Coverage remained the same at 47.217% when pulling d83e6c4 on atomic_transcript_migrate into 6798d80 on main.

@@ -108,8 +108,17 @@ def enqueue_async_migrate_transcripts_tasks(course_keys,
'command_run': command_run
}

from django.db import transaction
Copy link

Choose a reason for hiding this comment

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

should we keep imports together at the top of the file?

Copy link
Author

Choose a reason for hiding this comment

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

In general yes. I this case, I'm planning to revert this hack entirely, so it's worth to keep everything in a single place.

@github-actions

This comment has been minimized.

@OmarIthawi OmarIthawi marked this pull request as draft February 3, 2022 15:38
@github-actions

This comment has been minimized.

@OmarIthawi OmarIthawi marked this pull request as ready for review March 1, 2022 12:16
@OmarIthawi OmarIthawi force-pushed the atomic_transcript_migrate branch from cabde92 to 133df0a Compare March 1, 2022 12:23
@github-actions

This comment has been minimized.

@OmarIthawi OmarIthawi force-pushed the atomic_transcript_migrate branch from 133df0a to 15c4f1e Compare March 1, 2022 13:54
@github-actions

This comment has been minimized.

 - a course will either be successful or will fail in one atomic SQL
   transaction.
 - a course failure will be logged, then continue for next course
 - ItemNotFoundError will be ignored seperately from other errors since
   it's usually means that we have some other problem that
   the `migrate_transcripts` command shouldn't worry about
@OmarIthawi OmarIthawi force-pushed the atomic_transcript_migrate branch from 15c4f1e to d83e6c4 Compare March 1, 2022 14:17
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

Checking git merge conflicts against https://github.com/edx/edx-platform.git

Comparing with open-release/koa.master
Benchmark conflicts with main 101
Current conflicts 101
Summary Good work! No added conflicts.
Comparing with master
Benchmark conflicts with main 286
Current conflicts 286
Summary Good work! No added conflicts.

@OmarIthawi OmarIthawi merged commit ee287d6 into main Mar 1, 2022
@OmarIthawi OmarIthawi deleted the atomic_transcript_migrate branch March 1, 2022 14:58
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