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

Speed up first reservable time calculation #1252

Merged
merged 11 commits into from
Aug 6, 2024

Conversation

matti-lamppu
Copy link
Collaborator

@matti-lamppu matti-lamppu commented Jun 26, 2024

🛠️ Changelog

Speeds up first reservable time calculation in a two different ways:

  1. Pre-calculates affecting reservations with a materialized view. The view should be updated with a scheduled task.

  2. Run calculation for fewer units based on pagination arguments. Use a custom QuerySet.iterator() to fetch chunks of reservation units and their affecting reservations

The above optimizations achieve <1s fetching with our test data when calculating FRT, regardless of showOnlyReservable or any pagination args.

🧪 Test plan

  • Automated tests

🚧 Dependencies

  • Setup a periodic job in Django to refresh AffectingTimeSpans in platta envs.

🎫 Tickets

@matti-lamppu matti-lamppu added the improvement Improves an existing feature label Jun 26, 2024
@matti-lamppu matti-lamppu self-assigned this Jun 26, 2024
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch 4 times, most recently from bdb3188 to 98e2cec Compare June 26, 2024 12:14
@matti-lamppu matti-lamppu changed the base branch from main to fix-merge-overlapping-timespans June 27, 2024 06:06
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch 2 times, most recently from 271e9ed to 5d46e8c Compare June 27, 2024 06:38
Base automatically changed from fix-merge-overlapping-timespans to main June 27, 2024 07:02
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from 5d46e8c to c95d4c6 Compare June 27, 2024 07:39
@matti-lamppu matti-lamppu changed the base branch from main to fix-application-status-filters June 28, 2024 07:10
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from c95d4c6 to e67c6cf Compare June 28, 2024 07:11
Base automatically changed from fix-application-status-filters to main June 28, 2024 07:37
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from e67c6cf to 44f6c36 Compare June 28, 2024 09:29
@ranta ranta force-pushed the speed-up-first-reservable-time-calculation branch from 44f6c36 to 01979ab Compare July 9, 2024 19:23
@ranta ranta force-pushed the speed-up-first-reservable-time-calculation branch from 01979ab to 6d675d2 Compare July 28, 2024 16:28
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch 2 times, most recently from 70e582c to 8884c0a Compare August 2, 2024 13:16
Copy link

github-actions bot commented Aug 2, 2024

Triggered from #1252 by @​matti-lamppu.

Checking if we can fast forward main (1ee0c87) to speed-up-first-reservable-time-calculation (8884c0a).

Target branch (main):

commit 1ee0c8758afa804960d9424e0b7fd56124405d77 (HEAD -> main, origin/main, origin/HEAD)
Author: Matti Lamppu <matti.lamppu@vincit.fi>
Date:   Fri Aug 2 11:21:06 2024 +0300

    Switch to ECR for pulling docker images
    
    This is done due to hitting pull rate limits for Docker Hub
    (for unauthenticated users) in Azure Pipelines. At the time,
    this is 100 pulls per 6 hours, which is shared with all platta
    developers. ECR offers 1 pull per second (for unauthenticated users),
    which we shouldn't hit too often, since CI also has retries for
    pulling images.

Pull request (speed-up-first-reservable-time-calculation):

commit 8884c0a3bbf0c962c4de276b1885ca78ca22430b (pull_request/speed-up-first-reservable-time-calculation)
Author: Matti Lamppu <matti.lamppu@vincit.fi>
Date:   Fri Aug 2 16:14:47 2024 +0300

    Optimize affecting time spans fetching for FRT calc
    
    Instead of fetching affecting time span elements with a prefetch,
    create a custom QuerySet iterator method that can fetch the
    time spans in chunks when a new iterator chunk needs to be fetched.
    This way we also don't create so many duplicates for in memory.
    The method seems much faster (up to 4x), executing in less than a
    second with our current test dataset.

Can't fast forward main (1ee0c87) to speed-up-first-reservable-time-calculation (8884c0a). main (1ee0c87) is not a direct ancestor of speed-up-first-reservable-time-calculation (8884c0a). Branches appear to have diverged at 09c30da:

* 8884c0a3bbf0c962c4de276b1885ca78ca22430b Optimize affecting time spans fetching for FRT calc
* d51377d640b907248fbbadcb0b51b31de40199bb Use affecting tims spans for first reservable time calculation
* be9ba0a66b5b394b0c6d82f79e2cbf495cabf491 Use affecting time spans in recurring reservation generation
* 80b6a0bb9df63f67e6bacf92a37e63ca51f96b63 Add affecting time spans materialized view
| * 1ee0c8758afa804960d9424e0b7fd56124405d77 Switch to ECR for pulling docker images
| * ed7b8cfda646e6cf20de4d0016668ed4310f46a6 Add a warning for using test data creation in production environment
| * c26a4c0a2937ec6e5198672dd9d1ff6bfa2e48b5 Add `allow_cascade=True` to test data creation flush
|/  
* 09c30daeba5cdc89c9a8f97e5f0f9c0bc6ab7b15 Add another option for fast-forward merge comment

commit 09c30daeba5cdc89c9a8f97e5f0f9c0bc6ab7b15 (tag: v0.53.1)
Author: Matti Lamppu <matti.lamppu@vincit.fi>
Date:   Wed Jul 31 12:31:29 2024 +0300

    Add another option for fast-forward merge comment

Rebase locally, and then force push to speed-up-first-reservable-time-calculation.

@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from 8884c0a to 268640e Compare August 2, 2024 13:17
@matti-lamppu
Copy link
Collaborator Author

Idea: Maybe there should be a flag set in cache which indicates that the materialized view refresh is valid, in case the job fails for some reason. This would then force a refresh in the endpoint if the value is missing.

@matti-lamppu matti-lamppu marked this pull request as ready for review August 2, 2024 13:26
@matti-lamppu matti-lamppu requested review from vergama and ranta August 2, 2024 13:26
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from 268640e to 941d311 Compare August 5, 2024 07:32
@vergama
Copy link

vergama commented Aug 5, 2024

Idea: Maybe there should be a flag set in cache which indicates that the materialized view refresh is valid, in case the job fails for some reason. This would then force a refresh in the endpoint if the value is missing.

Or value of when the view was last updated; force refresh if too old?
Some complications may be caused by parallel endpoint hits encountering invalid mat.view, in which case the refresh should be synchronized...

Copy link

@vergama vergama left a comment

Choose a reason for hiding this comment

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

"The dude abides."

Copy link

github-actions bot commented Aug 6, 2024

Triggered from #1252 by @​matti-lamppu.

Checking if we can fast forward main (30dea12) to speed-up-first-reservable-time-calculation (35f3900).

Target branch (main):

commit 30dea125e7440ea3a5fa9c4d14fcb2e84a5df9f9 (HEAD -> main, origin/main, origin/HEAD)
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Mon Aug 5 21:16:47 2024 +0000

    [pre-commit.ci] pre-commit autoupdate
    
    updates:
    - [github.com/astral-sh/ruff-pre-commit: v0.5.5 → v0.5.6](https://github.com/astral-sh/ruff-pre-commit/compare/v0.5.5...v0.5.6)

Pull request (speed-up-first-reservable-time-calculation):

commit 35f3900f1f0294548376a51975e0fa760ac222f1 (pull_request/speed-up-first-reservable-time-calculation)
Author: Matti Lamppu <matti.lamppu@vincit.fi>
Date:   Tue Aug 6 07:44:57 2024 +0300

    Use a value in cache to check if AffectingTimeSpans are valid
    
    Set the value whenever the view is refreshed. Force a refresh
    in the FRT calculation if the view is invalid.

Can't fast forward main (30dea12) to speed-up-first-reservable-time-calculation (35f3900). main (30dea12) is not a direct ancestor of speed-up-first-reservable-time-calculation (35f3900). Branches appear to have diverged at 1ee0c87:

* 35f3900f1f0294548376a51975e0fa760ac222f1 Use a value in cache to check if AffectingTimeSpans are valid
* 941d31112ccba5b8f17f24814cc866182cb6cbae Optimize affecting time spans fetching for FRT calc
* 52bfdb202bade8ef33d00d2ef8da3e3d86579d98 Use affecting tims spans for first reservable time calculation
* 188152ebac64569f0560a6f47c8a573458908478 Use affecting time spans in recurring reservation generation
* 86dfb00172a8f7f51427cffcd799c5cd347740ea Add affecting time spans materialized view
| * 30dea125e7440ea3a5fa9c4d14fcb2e84a5df9f9 [pre-commit.ci] pre-commit autoupdate
| * 92cf2304d603ed0e93876db4030f836193811b79 build(deps): bump the python-dependencies group with 7 updates
|/  
* 1ee0c8758afa804960d9424e0b7fd56124405d77 Switch to ECR for pulling docker images

commit 1ee0c8758afa804960d9424e0b7fd56124405d77
Author: Matti Lamppu <matti.lamppu@vincit.fi>
Date:   Fri Aug 2 11:21:06 2024 +0300

    Switch to ECR for pulling docker images
    
    This is done due to hitting pull rate limits for Docker Hub
    (for unauthenticated users) in Azure Pipelines. At the time,
    this is 100 pulls per 6 hours, which is shared with all platta
    developers. ECR offers 1 pull per second (for unauthenticated users),
    which we shouldn't hit too often, since CI also has retries for
    pulling images.

Rebase locally, and then force push to speed-up-first-reservable-time-calculation.

Instead of fetching affecting time span elements with a prefetch,
create a custom QuerySet iterator method that can fetch the
time spans in chunks when a new iterator chunk needs to be fetched.
This way we also don't create so many duplicates for in memory.
The method seems much faster (up to 4x), executing in less than a
second with our current test dataset.
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from 35f3900 to c2992be Compare August 6, 2024 04:48
@matti-lamppu
Copy link
Collaborator Author

Added a value to cache for when the view was last updated. The FRT calculation will check this and refresh the view if it is too old. Refreshing the view with AffectingTimeSpan.refresh() updates the cached value.

Also, added error handling logic to the .refresh() methods on both materialized views to raise an error if the refresh fails during local development, but log to sentry in other environments.

Set the value whenever the view is refreshed. Force a refresh
in the FRT calculation if the view is invalid.
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from c2992be to 1432908 Compare August 6, 2024 05:06
@matti-lamppu
Copy link
Collaborator Author

Merged #1234, #1274, #1261 to this PR so that they can be merged to main together.

matti-lamppu and others added 6 commits August 6, 2024 13:50
- Reorder and group model fields
- Skip creating statistic during tests
- Small fixes to tests
- reservee_address_zip if reservation for profile user
- reservee_id & reservee_organisation_name if reservee not individual
@matti-lamppu matti-lamppu force-pushed the speed-up-first-reservable-time-calculation branch from 16e21e7 to 60e7771 Compare August 6, 2024 10:52
Copy link

sonarqubecloud bot commented Aug 6, 2024

@matti-lamppu
Copy link
Collaborator Author

/ff

Copy link

github-actions bot commented Aug 6, 2024

Triggered from #1252 (comment) by @​matti-lamppu.

Trying to fast forward main (30dea12) to speed-up-first-reservable-time-calculation (60e7771).

Target branch (main):

commit 30dea125e7440ea3a5fa9c4d14fcb2e84a5df9f9 (HEAD -> main, origin/main, origin/HEAD)
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Mon Aug 5 21:16:47 2024 +0000

    [pre-commit.ci] pre-commit autoupdate
    
    updates:
    - [github.com/astral-sh/ruff-pre-commit: v0.5.5 → v0.5.6](https://github.com/astral-sh/ruff-pre-commit/compare/v0.5.5...v0.5.6)

Pull request (speed-up-first-reservable-time-calculation):

commit 60e7771f302587389f8774ecfb7988478f2189cf (pull_request/speed-up-first-reservable-time-calculation)
Author: Eemeli Ranta <eemeli.ranta@vincit.fi>
Date:   Wed Jul 3 20:58:21 2024 +0300

    Update email feedback URL

Fast forwarding main (30dea12) to speed-up-first-reservable-time-calculation (60e7771).

$ git push origin 60e7771f302587389f8774ecfb7988478f2189cf:main
To https://github.com/City-of-Helsinki/tilavarauspalvelu-core.git
   30dea125..60e7771f  60e7771f302587389f8774ecfb7988478f2189cf -> main

@github-actions github-actions bot merged commit 60e7771 into main Aug 6, 2024
6 checks passed
@github-actions github-actions bot deleted the speed-up-first-reservable-time-calculation branch August 6, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants