From 8d5b597b69f9ea3641eb535a30bfcddf45373067 Mon Sep 17 00:00:00 2001 From: Daniel Ziegenberg Date: Mon, 19 May 2025 18:26:15 +0200 Subject: [PATCH] MDL-77593 communication: improve performance adding or removing members Before this change, the communication room gets all the users when enrolled or from the course when a new communication instance is added. It happens using an ad-hoc task but there is no efficiency to handle the number of users should be added in one go. This might have scaling issues in terms of courses with massive number of enrolment or users. This commit implements a logic to chunk the data and queue them so these can happen in smaller chunks without keeping the servers busy for a long time. Signed-off-by: Daniel Ziegenberg --- .../classes/task/add_members_to_room_task.php | 23 +++++++++-------- .../classes/task/remove_members_from_room.php | 25 +++++++++++-------- public/communication/tests/api_test.php | 16 ++++++------ 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/public/communication/classes/task/add_members_to_room_task.php b/public/communication/classes/task/add_members_to_room_task.php index f11cbc04bb5..09235c305d0 100644 --- a/public/communication/classes/task/add_members_to_room_task.php +++ b/public/communication/classes/task/add_members_to_room_task.php @@ -32,14 +32,14 @@ class add_members_to_room_task extends adhoc_task { $data = $this->get_custom_data(); // Call the communication api to action the operation. - $communication = processor::load_by_id($data->id); + $communication = processor::load_by_id($data->commid); if ($communication === null) { - mtrace("Skipping room creation because the instance does not exist"); + mtrace("Skipping adding members to room because the instance does not exist"); return; } - $communication->get_room_user_provider()->add_members_to_room($communication->get_instance_userids()); + $communication->get_room_user_provider()->add_members_to_room($data->userids); } /** @@ -51,13 +51,16 @@ class add_members_to_room_task extends adhoc_task { processor $communication ): void { - // Add ad-hoc task to update the provider room. - $task = new self(); - $task->set_custom_data([ - 'id' => $communication->get_id(), - ]); + foreach (array_chunk($communication->get_instance_userids(), 30) as $userids) { + // Add ad-hoc task to update the provider room. + $task = new self(); + $task->set_custom_data([ + 'commid' => $communication->get_id(), + 'userids' => $userids, + ]); - // Queue the task for the next run. - \core\task\manager::queue_adhoc_task($task); + // Queue the task for the next run. + \core\task\manager::queue_adhoc_task($task); + } } } diff --git a/public/communication/classes/task/remove_members_from_room.php b/public/communication/classes/task/remove_members_from_room.php index 1d6d444c3db..387ba07f5a1 100644 --- a/public/communication/classes/task/remove_members_from_room.php +++ b/public/communication/classes/task/remove_members_from_room.php @@ -32,21 +32,21 @@ class remove_members_from_room extends adhoc_task { $data = $this->get_custom_data(); // Call the communication api to action the operation. - $communication = processor::load_by_id($data->id); + $communication = processor::load_by_id($data->commid); if ($communication === null) { - mtrace("Skipping room creation because the instance does not exist"); + mtrace("Skipping removing members from room because the instance does not exist"); return; } - $communication->get_room_user_provider()->remove_members_from_room($communication->get_all_delete_flagged_userids()); + $communication->get_room_user_provider()->remove_members_from_room($data->userids); // Now remove any mapping for users who are not in the room. $communication->delete_instance_non_synced_user_mapping($communication->get_instance_userids(false, true)); } /** - * Queue the task for the next run. + * Queue the tasks for the next run. * * @param processor $communication The communication processor to perform the action on */ @@ -54,13 +54,16 @@ class remove_members_from_room extends adhoc_task { processor $communication ): void { - // Add ad-hoc task to update the provider room. - $task = new self(); - $task->set_custom_data([ - 'id' => $communication->get_id(), - ]); + foreach (array_chunk($communication->get_all_delete_flagged_userids(), 30) as $userids) { + // Add ad-hoc task to update the provider room. + $task = new self(); + $task->set_custom_data([ + 'commid' => $communication->get_id(), + 'userids' => $userids, + ]); - // Queue the task for the next run. - \core\task\manager::queue_adhoc_task($task); + // Queue the task for the next run. + \core\task\manager::queue_adhoc_task($task); + } } } diff --git a/public/communication/tests/api_test.php b/public/communication/tests/api_test.php index 6aeb4635f78..0151e6a4783 100644 --- a/public/communication/tests/api_test.php +++ b/public/communication/tests/api_test.php @@ -285,8 +285,10 @@ final class api_test extends \advanced_testcase { */ public function test_adding_and_removing_of_room_membership(): void { $course = $this->get_course(); - $userid = $this->get_user()->id; - + $userids = []; + for ($i = 0; $i < 40; $i++) { + $userids[] = $this->get_user('Samplefn' . $i, 'Sampleln' . $i, 'sampleun' . $i)->id; + } // First test the adding members to a room. $communication = \core_communication\api::load_by_instance( context: \core\context\course::instance($course->id), @@ -294,18 +296,18 @@ final class api_test extends \advanced_testcase { instancetype: 'coursecommunication', instanceid: $course->id, ); - $communication->add_members_to_room([$userid]); + $communication->add_members_to_room($userids); // Test the tasks added. $adhoctask = \core\task\manager::get_adhoc_tasks('\\core_communication\\task\\add_members_to_room_task'); - $this->assertCount(1, $adhoctask); + $this->assertCount(2, $adhoctask); // Now test the removing members from a room. - $communication->remove_members_from_room([$userid]); + $communication->remove_members_from_room($userids); // Test the tasks added. $adhoctask = \core\task\manager::get_adhoc_tasks('\\core_communication\\task\\remove_members_from_room'); - $this->assertCount(1, $adhoctask); + $this->assertCount(2, $adhoctask); } /** @@ -441,7 +443,7 @@ final class api_test extends \advanced_testcase { users: [$userid], ); - // Test that no communicaiton task is added. + // Test that no communication task is added. $adhoctask = \core\task\manager::get_adhoc_tasks(create_and_configure_room_task::class); $this->assertCount(0, $adhoctask);