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] V9 migration verification script optimisation - fetch ReadNotifications for account comparison #12905

Merged
merged 5 commits into from Mar 18, 2024

Conversation

NicolasCwy
Copy link
Contributor

@NicolasCwy NicolasCwy commented Mar 16, 2024

Part of #12048

Context
When comparing the SQL account to its datastore account for verification, multiple select query for each account was executed to get their readNotifications. This was be because the association between accounts and readNotifications is lazy.

Outline of Solution
Reduce readNotification fetches by fetching the data for a page using a single join, reducing the number of joins. (Multiple joins per account to 1 join for a page of accounts)
~1min2s for 10,000 accounts with 2 readNotifications to ~3 seconds
Fetching

Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

Since this PR is to fetch ReadNotif using join with Account, let's also change the PR title as it is misleading 🙏

CriteriaBuilder cb = HibernateUtil.getCriteriaBuilder();
CriteriaQuery<teammates.storage.sqlentity.Account> pageQuery = cb.createQuery(sqlEntityClass);

// sort by createdAt to maintain stable order.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// sort by createdAt to maintain stable order.
// sort by id to maintain stable order.

@NicolasCwy NicolasCwy changed the title [#12048] V9 migration verification script - join accountRequests to Accounts [#12048] V9 migration verification script optimisation - fetch ReadNotifications for account comparison Mar 17, 2024
Copy link
Contributor

@FergusMok FergusMok 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 benchmarking, adding the logging and helping to debug the problem!

Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@NicolasCwy NicolasCwy merged commit 868982f into TEAMMATES:master Mar 18, 2024
11 checks passed
@cedricongjh cedricongjh added this to the V9.0.0-beta.1 milestone Mar 24, 2024
@NicolasCwy NicolasCwy deleted the eager-load-relation branch April 11, 2024 06:54
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.

None yet

4 participants