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.

Sign up for GitHub

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

Jump to bottom

fix: Limits the events processed in the cron to avoid overflows and timeouts. #203

Merged
merged 6 commits into from
Aug 1, 2018
Merged

fix: Limits the events processed in the cron to avoid overflows and timeouts. #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cbd2646
Update store.php
AndyHubert Jul 30, 2018
770d3ed
Update emit_task.php
AndyHubert Jul 30, 2018
478d8b8
Update store.php
AndyHubert Jul 30, 2018
44d0345
Update emit_task.php
AndyHubert Jul 31, 2018
21b6496
Update emit_task.php
AndyHubert Jul 31, 2018
45bd85f
Merge branch 'master' into patch-4
ryasmi Aug 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion classes/log/store.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ protected function insert_event_entries(array $events) {
}
}

public function get_max_batch_size() {
return $this->get_config('maxbatchsize', 100);
}

public function process_events(array $events) {
global $DB;
global $CFG;
Expand Down Expand Up @@ -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(),
Copy link
Contributor

@caperneoignis caperneoignis Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

],
];
$loadedevents = \src\handler($handlerconfig, $events);
Expand Down
7 changes: 6 additions & 1 deletion classes/task/emit_task.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@ public function execute() {
global $DB;
$manager = get_log_manager();
$store = new store($manager);
$extractedevents = $DB->get_records('logstore_xapi_log');
$conditions = null;
$sort = '';
$fields = '*';
$limitfrom = 0;
$limitnum = $store->get_max_batch_size();
$extractedevents = $DB->get_records('logstore_xapi_log', $conditions, $sort, $fields, $limitfrom, $limitnum);
$loadedevents = $store->process_events($extractedevents);
$loadedeventids = array_map(function ($transformedevent) {
return $transformedevent['eventid'];
Expand Down