From 8736fbc19003d089a434f9b013d04c34d63703ec Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 6 Dec 2017 18:17:44 +0000 Subject: [PATCH] MDL-60981 core_search: New indexpriority field in search index queue Adds indexpriority field to the database table which holds a queue of indexing requests. This allows for potentially large area reindexes to have a lower priority, so as not to halt the special indexes that run after a course restore. --- lib/db/install.xml | 6 ++- lib/db/upgrade.php | 33 +++++++++++++++++ search/classes/manager.php | 50 ++++++++++++++++++++----- search/tests/manager_test.php | 70 +++++++++++++++++++++++++++++++---- version.php | 2 +- 5 files changed, 143 insertions(+), 18 deletions(-) diff --git a/lib/db/install.xml b/lib/db/install.xml index 27c832ae105..593ff37a451 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -3770,11 +3770,15 @@ + + + + \ No newline at end of file diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index e2e68c1526b..504fbfdafab 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -1902,5 +1902,38 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2017121900.00); } + if ($oldversion < 2017122200.01) { + + // Define field indexpriority to be added to search_index_requests. Allow null initially. + $table = new xmldb_table('search_index_requests'); + $field = new xmldb_field('indexpriority', XMLDB_TYPE_INTEGER, '10', + null, null, null, null, 'partialtime'); + + // Conditionally add field. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + + // Set existing values to 'normal' value (100). + $DB->set_field('search_index_requests', 'indexpriority', 100); + + // Now make the field 'NOT NULL'. + $field = new xmldb_field('indexpriority', XMLDB_TYPE_INTEGER, '10', + null, XMLDB_NOTNULL, null, null, 'partialtime'); + $dbman->change_field_notnull($table, $field); + } + + // Define index indexprioritytimerequested (not unique) to be added to search_index_requests. + $index = new xmldb_index('indexprioritytimerequested', XMLDB_INDEX_NOTUNIQUE, + array('indexpriority', 'timerequested')); + + // Conditionally launch add index indexprioritytimerequested. + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2017122200.01); + } + return true; } diff --git a/search/classes/manager.php b/search/classes/manager.php index dd77270cf12..2b131f40e89 100644 --- a/search/classes/manager.php +++ b/search/classes/manager.php @@ -87,6 +87,16 @@ class manager { */ const DISPLAY_INDEXING_PROGRESS_EVERY = 30.0; + /** + * @var int Context indexing: normal priority. + */ + const INDEX_PRIORITY_NORMAL = 100; + + /** + * @var int Context indexing: low priority for reindexing. + */ + const INDEX_PRIORITY_REINDEXING = 50; + /** * @var \core_search\base[] Enabled search areas. */ @@ -1147,32 +1157,50 @@ class manager { * * @param \context $context Context to index within * @param string $areaid Area to index, '' = all areas + * @param int $priority Priority (INDEX_PRIORITY_xx constant) */ - public static function request_index(\context $context, $areaid = '') { + public static function request_index(\context $context, $areaid = '', + $priority = self::INDEX_PRIORITY_NORMAL) { global $DB; // Check through existing requests for this context or any parent context. list ($contextsql, $contextparams) = $DB->get_in_or_equal( $context->get_parent_context_ids(true)); $existing = $DB->get_records_select('search_index_requests', - 'contextid ' . $contextsql, $contextparams, '', 'id, searcharea, partialarea'); + 'contextid ' . $contextsql, $contextparams, '', + 'id, searcharea, partialarea, indexpriority'); foreach ($existing as $rec) { // If we haven't started processing the existing request yet, and it covers the same // area (or all areas) then that will be sufficient so don't add anything else. if ($rec->partialarea === '' && ($rec->searcharea === $areaid || $rec->searcharea === '')) { - return; + // If the existing request has the same (or higher) priority, no need to add anything. + if ($rec->indexpriority >= $priority) { + return; + } + // The existing request has lower priority. If it is exactly the same, then just + // adjust the priority of the existing request. + if ($rec->searcharea === $areaid) { + $DB->set_field('search_index_requests', 'indexpriority', $priority, + ['id' => $rec->id]); + return; + } + // The existing request would cover this area but is a lower priority. We need to + // add the new request even though that means we will index part of it twice. } } // No suitable existing request, so add a new one. $newrecord = [ 'contextid' => $context->id, 'searcharea' => $areaid, - 'timerequested' => time(), 'partialarea' => '', 'partialtime' => 0 ]; + 'timerequested' => (int)self::get_current_time(), + 'partialarea' => '', 'partialtime' => 0, + 'indexpriority' => $priority ]; $DB->insert_record('search_index_requests', $newrecord); } /** - * Processes outstanding index requests. This will take the first item from the queue and - * process it, continuing until an optional time limit is reached. + * Processes outstanding index requests. This will take the first item from the queue (taking + * account the indexing priority) and process it, continuing until an optional time limit is + * reached. * * If there are no index requests, the function will do nothing. * @@ -1186,7 +1214,6 @@ class manager { $progress = new \null_progress_trace(); } - $complete = false; $before = self::get_current_time(); if ($timelimit) { $stopat = $before + $timelimit; @@ -1194,11 +1221,10 @@ class manager { while (true) { // Retrieve first request, using fully defined ordering. $requests = $DB->get_records('search_index_requests', null, - 'timerequested, contextid, searcharea', + 'indexpriority DESC, timerequested, contextid, searcharea', 'id, contextid, searcharea, partialarea, partialtime', 0, 1); if (!$requests) { // If there are no more requests, stop. - $complete = true; break; } $request = reset($requests); @@ -1208,6 +1234,12 @@ class manager { $beforeindex = self::get_current_time(); if ($timelimit) { $remainingtime = $stopat - $beforeindex; + + // If the time limit expired already, stop now. (Otherwise we might accidentally + // index with no time limit or a negative time limit.) + if ($remainingtime <= 0) { + break; + } } // Show a message before each request, indicating what will be indexed. diff --git a/search/tests/manager_test.php b/search/tests/manager_test.php index 3f1c62147d5..b64cb159929 100644 --- a/search/tests/manager_test.php +++ b/search/tests/manager_test.php @@ -941,6 +941,39 @@ class search_manager_testcase extends advanced_testcase { // if we had already begun processing the previous entry.) \core_search\manager::request_index($forum2ctx); $this->assertEquals(4, $DB->count_records('search_index_requests')); + + // Clear queue and do tests relating to priority. + $DB->delete_records('search_index_requests'); + + // Request forum 1, specific area, priority 100. + \core_search\manager::request_index($forum1ctx, 'forum-post', 100); + $results = array_values($DB->get_records('search_index_requests', null, 'id')); + $this->assertCount(1, $results); + $this->assertEquals(100, $results[0]->indexpriority); + + // Request forum 1, same area, lower priority; no change. + \core_search\manager::request_index($forum1ctx, 'forum-post', 99); + $results = array_values($DB->get_records('search_index_requests', null, 'id')); + $this->assertCount(1, $results); + $this->assertEquals(100, $results[0]->indexpriority); + + // Request forum 1, same area, higher priority; priority stored changes. + \core_search\manager::request_index($forum1ctx, 'forum-post', 101); + $results = array_values($DB->get_records('search_index_requests', null, 'id')); + $this->assertCount(1, $results); + $this->assertEquals(101, $results[0]->indexpriority); + + // Request forum 1, all areas, lower priority; adds second entry. + \core_search\manager::request_index($forum1ctx, '', 100); + $results = array_values($DB->get_records('search_index_requests', null, 'id')); + $this->assertCount(2, $results); + $this->assertEquals(100, $results[1]->indexpriority); + + // Request course 1, all areas, lower priority; adds third entry. + \core_search\manager::request_index($course1ctx, '', 99); + $results = array_values($DB->get_records('search_index_requests', null, 'id')); + $this->assertCount(3, $results); + $this->assertEquals(99, $results[2]->indexpriority); } /** @@ -969,14 +1002,15 @@ class search_manager_testcase extends advanced_testcase { // Hack the forums so they have different creation times. $now = time(); - $DB->set_field('forum', 'timemodified', $now - 3, ['id' => $forum1->id]); - $DB->set_field('forum', 'timemodified', $now - 2, ['id' => $forum2->id]); - $DB->set_field('forum', 'timemodified', $now - 1, ['id' => $forum3->id]); - $forum2time = $now - 2; + $DB->set_field('forum', 'timemodified', $now - 30, ['id' => $forum1->id]); + $DB->set_field('forum', 'timemodified', $now - 20, ['id' => $forum2->id]); + $DB->set_field('forum', 'timemodified', $now - 10, ['id' => $forum3->id]); + $forum2time = $now - 20; // Make 2 index requests. + testable_core_search::fake_current_time($now - 3); $search::request_index(context_course::instance($course->id), 'mod_label-activity'); - $this->waitForSecond(); + testable_core_search::fake_current_time($now - 2); $search::request_index(context_module::instance($forum1->cmid)); // Run with no time limit. @@ -998,12 +1032,13 @@ class search_manager_testcase extends advanced_testcase { $this->assertEquals(0, $DB->count_records('search_index_requests')); // Request indexing the course a couple of times. + testable_core_search::fake_current_time($now - 3); $search::request_index(context_course::instance($course->id), 'mod_forum-activity'); + testable_core_search::fake_current_time($now - 2); $search::request_index(context_course::instance($course->id), 'mod_forum-post'); // Do the processing again with a time limit and indexing delay. The time limit is too // small; because of the way the logic works, this means it will index 2 activities. - testable_core_search::fake_current_time(time()); $search->get_engine()->set_add_delay(0.2); $search->process_index_requests(0.1, $progress); $out = $progress->get_buffer(); @@ -1024,7 +1059,7 @@ class search_manager_testcase extends advanced_testcase { $this->assertEquals($forum2time, $records[0]->partialtime); // Run again and confirm it now finishes. - $search->process_index_requests(0.1, $progress); + $search->process_index_requests(2.0, $progress); $out = $progress->get_buffer(); $progress->reset_buffer(); $this->assertContains( @@ -1036,5 +1071,26 @@ class search_manager_testcase extends advanced_testcase { // Confirm table is now empty. $this->assertEquals(0, $DB->count_records('search_index_requests')); + + // Make 2 requests - first one is low priority. + testable_core_search::fake_current_time($now - 3); + $search::request_index(context_module::instance($forum1->cmid), 'mod_forum-activity', + \core_search\manager::INDEX_PRIORITY_REINDEXING); + testable_core_search::fake_current_time($now - 2); + $search::request_index(context_module::instance($forum2->cmid), 'mod_forum-activity'); + + // Process with short time limit and confirm it does the second one first. + $search->process_index_requests(0.1, $progress); + $out = $progress->get_buffer(); + $progress->reset_buffer(); + $this->assertContains( + 'Completed requested context: Forum: TForum2 (search area: mod_forum-activity)', + $out); + $search->process_index_requests(0.1, $progress); + $out = $progress->get_buffer(); + $progress->reset_buffer(); + $this->assertContains( + 'Completed requested context: Forum: TForum1 (search area: mod_forum-activity)', + $out); } } diff --git a/version.php b/version.php index c2c7a6fc596..f9d19d2b84b 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2017122200.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2017122200.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.