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.

Sign up for GitHub

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

Jump to bottom

[#12048] Data migration for feedback session entities #12986

Merged
merged 6 commits into from
Apr 7, 2024
Merged

[#12048] Data migration for feedback session entities #12986

merged 6 commits into from
Apr 7, 2024

Conversation

yuanxi1
Copy link
Contributor

@yuanxi1 yuanxi1 commented Apr 6, 2024 •
edited
Loading

Part of #12048

Outline of Solution

  • Added migration and verification script for feedback session.
  • Added function to seed feedback sessions in SeedDb.java

Edit:

  • Migration script initially took very long due to the need to fetch the course reference and set it in the new feedback session entity.

  • Added a function getReference(entity class, id) in Hibernate.util to use EntityManager.getReference and use this instead of the normal get. Increased performance from nearly 1 hr to 1 minute for 30K entities.

  • Stats (tested on Google cloud):

No of entities tested Seed time Migration time Verification time Verify counts time (course + fb session)
30000 aprox 1hr in total 1m 12s (Without @Fetch annotation) 31.558s (With@Fetch annotation) TBD, > 30min 13s

@yuanxi1 yuanxi1 requested review from mingyuanc and NicolasCwy April 6, 2024 02:33
NicolasCwy
NicolasCwy requested changes Apr 6, 2024
View reviewed changes
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.

Nice work on this! I've highlighted 2 parts which could be causing verification to be slow

I think in this PR you can uncomment the seed (unless there's a reason) and also optimise your seed script

src/client/java/teammates/client/scripts/sql/DataMigrationForFeedbackSessionSql.java
protected void migrateEntity(FeedbackSession oldEntity) throws Exception {
HibernateUtil.beginTransaction();
Course course = HibernateUtil.get(teammates.storage.sqlentity.Course.class, oldEntity.getCourseId());
HibernateUtil.commitTransaction();
Copy link
Contributor

Choose a reason for hiding this comment

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

This transaction is probably causing the migration to be slow, we probably can optimise this by joining courses with its feedback sessions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to use EntityManager.getReference.

src/client/java/teammates/client/scripts/sql/SeedDb.java Outdated Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyFeedbackSessionAttributes.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyFeedbackSessionAttributes.java Show resolved Hide resolved
@NicolasCwy
Copy link
Contributor

NicolasCwy commented Apr 6, 2024 •
edited
Loading

@yuanxi1 Just realised you are also missing verifying entity counts could you make something similar to VerifyNonCourseEntityCounts with course counts too! Realised we missed it out in the course entity migration PR #12980

NicolasCwy
NicolasCwy approved these changes Apr 6, 2024
View reviewed changes
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! Nice work optimising migration using FK

@NicolasCwy NicolasCwy added s.FinalReview The PR is ready for final review c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Apr 6, 2024
@NicolasCwy NicolasCwy mentioned this pull request Apr 6, 2024
Closed
@NicolasCwy NicolasCwy force-pushed the v9-course-migration branch from 0e31828 to 82d0605 Compare April 7, 2024 03:47
@NicolasCwy NicolasCwy changed the title [#12048] Data migration for feedback session [#12048] Data migration for feedback session entities Apr 7, 2024
mingyuanc
mingyuanc reviewed Apr 7, 2024
View reviewed changes
Copy link
Contributor

@mingyuanc mingyuanc left a comment

Choose a reason for hiding this comment

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

Other than that, lgtm

src/client/java/teammates/client/scripts/sql/SeedDb.java
getRandomInstant(),
rand.nextInt(3) > 1 ? null : getRandomInstant(), // set deletedAt randomly at 25% chance
false);
ofy().save().entities(course).now();
} catch (Exception e) {
log(e.toString());
}

seedFeedbackSession(courseId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, should this be in the try block? so that if there is an exception then you wont seed the feedback session

Copy link
Contributor

@NicolasCwy NicolasCwy Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this case I think each seed function should not be catching the error within their own function but throwing to be caught here, let's make a note of this and fix it in another PR to unblock everyone else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, i'll change in feedback question pr then.

@NicolasCwy NicolasCwy merged commit bb58c58 into TEAMMATES:v9-course-migration Apr 7, 2024
1 check passed
@NicolasCwy NicolasCwy mentioned this pull request Apr 7, 2024
Merged
@NicolasCwy NicolasCwy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingyuanc mingyuanc mingyuanc left review comments

@NicolasCwy NicolasCwy NicolasCwy approved these changes

@FergusMok FergusMok FergusMok left review comments

@marquestye marquestye Awaiting requested review from marquestye

Assignees

@yuanxi1 yuanxi1

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuanxi1 @NicolasCwy @FergusMok @mingyuanc