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

[7806] plans and projects api caching #5372

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

m4ra
Copy link
Contributor

@m4ra m4ra commented Dec 14, 2023

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch 3 times, most recently from 911021b to 5b9fce3 Compare December 18, 2023 17:11
@m4ra m4ra changed the title WIP [7806] plans and projects api caching [7806] plans and projects api caching Dec 18, 2023
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Some initial comments and the question about how we handle cache expiry times

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch from 5b9fce3 to 067f03f Compare December 19, 2023 16:10
@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch 2 times, most recently from dba0904 to 5d9bab2 Compare December 21, 2023 15:22
@m4ra m4ra requested a review from goapunk December 21, 2023 15:22
@m4ra
Copy link
Contributor Author

m4ra commented Dec 21, 2023

@goapunk have another look when you can. I need to add a test, e.g adding future projects and check if the are added in the cache.

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch from 5d9bab2 to ea0e82f Compare December 21, 2023 15:36
@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch from ea0e82f to 60ea57c Compare January 8, 2024 14:53
@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch 5 times, most recently from 43f821a to 5f21b64 Compare January 23, 2024 11:17
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me so far, I left some comments with questions. I'm not sure about the failing tests, seems to be a bit tricky with the combination of redis + CELERY_TASK_ALWAYS_EAGER=True. Without it we need a running celery worker though

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch 3 times, most recently from 06608e7 to 5e82acf Compare January 23, 2024 21:54
@m4ra
Copy link
Contributor Author

m4ra commented Jan 24, 2024

@goapunk there was a typo in the reset cache task of the key to be deleted. As I was testing locally and because of the asterisks I was using, something was not captured. Anyhow I simplified the tests and removed the API calls as these are part of the test_api_caching. Also I ended up using a 10min check and a list of projects that will become past same as for future projects, otherwise we may miss any other expiring between the 10mins if one is already set for expiration. Looking again at the tasks and functions I wonder if they should be consolidate instead of having separated for future and past projects.

@goapunk
Copy link
Contributor

goapunk commented Jan 24, 2024

@goapunk there was a typo in the reset cache task of the key to be deleted. As I was testing locally and because of the asterisks I was using, something was not captured. Anyhow I simplified the tests and removed the API calls as these are part of the test_api_caching. Also I ended up using a 10min check and a list of projects that will become past same as for future projects, otherwise we may miss any other expiring between the 10mins if one is already set for expiration. Looking again at the tasks and functions I wonder if they should be consolidate instead of having separated for future and past projects.

I guess the only difference is the deletion of projects_futureParticipation vs projects_pastParticipation, so yeah, maybe we just delete them both and consolidate the code? What do you think?

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch from 5e82acf to 669d23d Compare January 25, 2024 10:04
@m4ra m4ra requested a review from goapunk January 25, 2024 10:11
@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch 3 times, most recently from 3a8f7f6 to 586fcd5 Compare January 25, 2024 11:49
@m4ra
Copy link
Contributor Author

m4ra commented Jan 25, 2024

@goapunk I merged the two scheduled tasks, the actual resets for future/past projects i left as it is, since it will save us from queering either future or past projects again via the url, which are time/power consuming.
I added also django query checks in the tests, very helpful and eye opening, as without cache, the api endpoint queries 12 times (with the prefetch-related).

Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Really nice! Just some small comments left

@goapunk
Copy link
Contributor

goapunk commented Jan 25, 2024

@goapunk I merged the two scheduled tasks, the actual resets for future/past projects i left as it is, since it will save us from queering either future or past projects again via the url, which are time/power consuming. I added also django query checks in the tests, very helpful and eye opening, as without cache, the api endpoint queries 12 times (with the prefetch-related).

@m4ra yeah, the query check is really helpful, we definitely need to work on improving the queries/structure. Ideally we will have checks in all our tests in the future 🤞

@m4ra m4ra force-pushed the mk-2023-12-refactor-api-caching branch from 586fcd5 to c594f32 Compare January 25, 2024 14:26
@m4ra
Copy link
Contributor Author

m4ra commented Jan 25, 2024

@goapunk done!

@m4ra m4ra requested a review from goapunk January 25, 2024 14:27
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Really cool :) Before we deploy it we need to make some changes in the admin repo

@goapunk goapunk merged commit 5b4fa19 into main Jan 25, 2024
2 checks passed
@goapunk goapunk deleted the mk-2023-12-refactor-api-caching branch January 25, 2024 15:23
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.

3 participants