fix: Limits the events processed in the cron to avoid overflows and timeouts. #203
Conversation
There was a problem hiding this comment.
Hey @AndyHubert, thanks for this, really appreciate it. Just one change to request.
classes/task/emit_task.php
Outdated
| @@ -39,7 +39,7 @@ public function execute() { | |||
| global $DB; | |||
| $manager = get_log_manager(); | |||
| $store = new store($manager); | |||
| $extractedevents = $DB->get_records('logstore_xapi_log'); | |||
| $extractedevents = $DB->get_records('logstore_xapi_log', null, '', '*', 0, $store->get_max_batch_size()); | |||
There was a problem hiding this comment.
Hey @AndyHubert, could you please extract these other arguments (null, '', '*', and 0) into variables so it's a little more obvious what they mean? For example the '*' might be defined as $selectedfields = '*' or something along those lines.
ryasmi
changed the title
Sure. I used the names found here: https://docs.moodle.org/dev/Data_manipulation_API#moodle_database::get_records.28.29
removed ending space
| @@ -108,7 +112,7 @@ public function process_events(array $events) { | |||
| 'lrs_endpoint' => $this->get_config('endpoint', ''), | |||
| 'lrs_username' => $this->get_config('username', ''), | |||
| 'lrs_password' => $this->get_config('password', ''), | |||
| 'lrs_max_batch_size' => $this->get_config('maxbatchsize', 100), | |||
| 'lrs_max_batch_size' => $this->get_max_batch_size(), | |||
There was a problem hiding this comment.
Real quick, do these event's get processed from oldest to newest or newest to oldest? Because if it's newest to oldest, the batch size could be small enough, that we start to get a backlog in bigger systems, under heavier usage. So we end up with events, never getting sent, because the amount of new events is greater then the batch size per cron run. Especially with systems seeing 1k users worth of traffic. Just a curious question.
There was a problem hiding this comment.
Looking at the code, I believe it would be oldest to newest. I'm going to merge this and we can resolve that issue separately when needed.
|
Thanks for adding those variables @AndyHubert, much easier to read that bit now 👍 |
|
🎉 This PR is included in version 3.9.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
If the backlog in the logstore_xapi_log gets too long, then the call to get_records can either time out or runs out of memory.