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

[#12048] Data migration for course entities #12980

Merged

Conversation

mingyuanc
Copy link
Contributor

@mingyuanc mingyuanc commented Apr 3, 2024

Part of #12048

Outline of Solution:
Abstracted away seed functions
Add migrating and verification scripts for course entity

Entity Test counts Migration Time Verification Time (Singapore)
Course 10000 27s 8.411s

Copy link

github-actions bot commented Apr 3, 2024

Hi @mingyuanc, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]
  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

@mingyuanc mingyuanc force-pushed the data-migration-course branch from 3097e6e to 1efd038 Compare April 3, 2024 15:48
@mingyuanc mingyuanc changed the title Data migration course [#12048] Migrating course entity Apr 3, 2024
@mingyuanc mingyuanc changed the base branch from master to v9-course-migration April 4, 2024 06:30
@mingyuanc mingyuanc requested review from NicolasCwy and yuanxi1 April 4, 2024 06:30
Comment on lines 38 to 44
protected boolean isMigrationNeeded(Course entity) {
HibernateUtil.beginTransaction();
teammates.storage.sqlentity.Course course = HibernateUtil.get(
teammates.storage.sqlentity.Course.class, entity.getUniqueId());
HibernateUtil.commitTransaction();
return course == null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@NicolasCwy
Is it gonna be costly to query the sql db for every entity here?
Also do we need a MarkIsMigrated script for Course like the MarkIsMigratedForAccounts.java since Course has a isMigrated field? If needed then maybe can just check if !isMigrated ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now we can comment the isMigratedNeeded body and return True @mingyuanc, let's get the scripts down then worry about improvements later

@yuanxi1 We'll see what the requirements are for migrating on prod and add this later if need be, for now you can just keep that script in mind if you need it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@NicolasCwy
Copy link
Contributor

@mingyuanc Please fix the lint errors! So that when we merge to master we won't cause other CI to fail

@mingyuanc mingyuanc requested a review from yuanxi1 April 4, 2024 15:20
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

LGTM thanks for getting this done so fast. Have you tested this locally to verify the migration and also the verification?

If so once the CI checks pass go ahead and merge it

@NicolasCwy NicolasCwy merged commit 44f3e0b into TEAMMATES:v9-course-migration Apr 4, 2024
1 check passed
@NicolasCwy NicolasCwy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Apr 6, 2024
@NicolasCwy NicolasCwy changed the title [#12048] Migrating course entity [#12048] Data migration for course entities Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants