-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: caching was removed #33617
fix: caching was removed #33617
Conversation
Thanks for the pull request, @Inferato! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Thanks for fixing this issue. Now, I'd like to know the performance impact this could have in the long run. Have you folks tested this with a more significant volume of users? Can the caching strategy time be set up to be lowered instead of removing it altogether? |
@Lunyachek - do you know if this is still being pursued? |
@mphilbrick211 I want to update and finalize the PR, but I have a problem with modifying this PR. @mariajgrimaldi You're right, we have checked it on courses with around 1000 users, and removing this caching might affect the performance on the page, we decided to just set the default caching to 10 minutes (600 seconds). I will reopen the same change in a new PR. |
@GlugovGrGlib I`m able to make these changes if it is ok for you. |
@GlugovGrGlib: As you mentioned, if merged to quince.master, it won't be included in a new release. But folks could still use quince.master branch. If that works for you, I could review the PR. Let me know! |
@Inferato @GlugovGrGlib: can we close this PR in favor of #34584? |
@mariajgrimaldi yes, I'm closing it now! |
@Inferato Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Caching is removed to represent an actual count of learners.
Related: open-release/quince.master