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

fix: caching was removed #33617

Closed

Conversation

Inferato
Copy link
Contributor

@Inferato Inferato commented Oct 30, 2023

Description

Caching is removed to represent an actual count of learners.

изображение

Related: open-release/quince.master

@openedx-webhooks
Copy link

openedx-webhooks commented Oct 30, 2023

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 30, 2023
@mariajgrimaldi
Copy link
Member

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?

@mphilbrick211
Copy link

@Lunyachek - do you know if this is still being pursued?

@GlugovGrGlib
Copy link
Member

GlugovGrGlib commented Apr 23, 2024

@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.
@mariajgrimaldi do you think opening PR against quince.master is still necessary since there won't be quince.4?

@Inferato
Copy link
Contributor Author

@GlugovGrGlib I`m able to make these changes if it is ok for you.

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Apr 24, 2024

@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!

@mariajgrimaldi
Copy link
Member

@Inferato @GlugovGrGlib: can we close this PR in favor of #34584?

@GlugovGrGlib
Copy link
Member

@mariajgrimaldi yes, I'm closing it now!

@openedx-webhooks
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants