Skip to content

Performance cherry picks#8

Merged
rhell4 merged 2 commits intoMOODLE_401_STABLEfrom
perf-fix-41
Aug 27, 2025
Merged

Performance cherry picks#8
rhell4 merged 2 commits intoMOODLE_401_STABLEfrom
perf-fix-41

Conversation

@matthewhilton
Copy link

@matthewhilton matthewhilton commented Aug 26, 2025

Background

  • Client discovered emailing task is running out of memory
  • Upstream codebase https://github.com/mdjnelson/moodle-mod_customcert has a few performance fixes here that we want, namely:
    • Number of certificates per run (instead of all)
    • Splitting emailing into an adhoc task
  • we don't want this to cause an outage, so we cannot touch the db dir - this unfortunately means we cannot directly cherry pick from Mark's repo as the commits aren't clean and some touch the db dir.

Note, this is intended to be a temporary patch until the client moves off this fork (soon!)

Process

  • First, I reverted PeterB's email performance fixes, as they are no longer necessary / superseded by these new improvements.
  • Then I went to the MOODLE_500_STABLE branch and copied the 2 tasks + unit test for them and pasted them in here
    • Note I purposely used the latest branch as it has some bugfixes that the MOODLE_401_STABLE branch didn't have.
  • I then renamed issue_certificates_task -> email_certificate_task (to allow db/tasks.php to be the same) and renamed email_certificate_task -> email_certificate_adhoc_task
  • I renamed references to these in the tests + codebase
  • Copied the necessary admin settings and lang strings
  • Changed \core\cron::set_user() -> $this->set_userid (5.0 vs 4.1 API difference)
  • Discovered 1 bug where adhoc tasks are not de-duplicated - so I added checkforexisting=true to the adhoc task queueing.
  • Minor bumped the version (due to new lang strings)

Risks

  • This should be fairly safe, the only risk would be if any other class changed and I did not copy those changes, however, from inspecting it I am fairly certain the only relevant changes are inside these two task classes

Testing

  • Unit tests - passing
  • Local testing - emailing works, task are de-duplicated as expected.

@rhell4
Copy link

rhell4 commented Aug 27, 2025

Everything looks good to me, there are unit tests that cover the change and everything is green.

@rhell4 rhell4 merged commit 8555873 into MOODLE_401_STABLE Aug 27, 2025
8 checks passed
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.

2 participants