From 1da7573bb77beff9e4466430b57551986e6be248 Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Wed, 16 Jan 2019 13:51:54 -0800 Subject: [PATCH] WatchedItemStore: Use batching in setNotificationTimestampsForUser Update rows in batches, using the same logic as is used by removeWatchBatchForUser(). Also remove the functionality for updating all rows, and move that to resetAllNotificationTimestampsForUser() instead. To that end, add a timestamp parameter to that method and to the job it uses, and make setNotificationTimestampsForUser() behave like a backwards-compatibility wrapper around resetAllNotificationTimestampsForUser() when no list of titles is specified. Bug: T207941 Change-Id: I58342257395de6fcfb4c392b3945b12883ca1680 Follows-Up: I2008ff89c95fe6f66a3fd789d2cef0e8fe52bd93 --- includes/api/ApiSetNotificationTimestamp.php | 9 +-- .../jobs/ClearWatchlistNotificationsJob.php | 61 ++++++++++----- includes/watcheditem/WatchedItemStore.php | 74 ++++++++++++++----- .../WatchedItemStoreIntegrationTest.php | 18 ++++- .../watcheditem/WatchedItemStoreUnitTest.php | 65 +++++++--------- 5 files changed, 137 insertions(+), 90 deletions(-) diff --git a/includes/api/ApiSetNotificationTimestamp.php b/includes/api/ApiSetNotificationTimestamp.php index c9ebfa88cc..ba4c6e8321 100644 --- a/includes/api/ApiSetNotificationTimestamp.php +++ b/includes/api/ApiSetNotificationTimestamp.php @@ -108,14 +108,7 @@ class ApiSetNotificationTimestamp extends ApiBase { $result = []; if ( $params['entirewatchlist'] ) { // Entire watchlist mode: Just update the thing and return a success indicator - if ( is_null( $timestamp ) ) { - $watchedItemStore->resetAllNotificationTimestampsForUser( $user ); - } else { - $watchedItemStore->setNotificationTimestampsForUser( - $user, - $timestamp - ); - } + $watchedItemStore->resetAllNotificationTimestampsForUser( $user, $timestamp ); $result['notificationtimestamp'] = is_null( $timestamp ) ? '' diff --git a/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php b/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php index 94c1351a79..b71580a525 100644 --- a/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php +++ b/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php @@ -22,11 +22,13 @@ use MediaWiki\MediaWikiServices; /** - * Job for clearing all of the "last viewed" timestamps for a user's watchlist + * Job for clearing all of the "last viewed" timestamps for a user's watchlist, or setting them all + * to the same value. * * Job parameters include: * - userId: affected user ID [required] * - casTime: UNIX timestamp of the event that triggered this job [required] + * - timestamp: value to set all of the "last viewed" timestamps to [optional, defaults to null] * * @ingroup JobQueue * @since 1.31 @@ -38,7 +40,7 @@ class ClearWatchlistNotificationsJob extends Job { static $required = [ 'userId', 'casTime' ]; $missing = implode( ', ', array_diff( $required, array_keys( $this->params ) ) ); if ( $missing != '' ) { - throw new InvalidArgumentException( "Missing paramter(s) $missing" ); + throw new InvalidArgumentException( "Missing parameter(s) $missing" ); } $this->removeDuplicates = true; @@ -51,29 +53,48 @@ class ClearWatchlistNotificationsJob extends Job { $dbw = $lbFactory->getMainLB()->getConnection( DB_MASTER ); $ticket = $lbFactory->getEmptyTransactionTicket( __METHOD__ ); + $timestamp = $this->params['timestamp'] ?? null; + if ( $timestamp === null ) { + $timestampCond = 'wl_notificationtimestamp IS NOT NULL'; + } else { + $timestamp = $dbw->timestamp( $timestamp ); + $timestampCond = 'wl_notificationtimestamp != ' . $dbw->addQuotes( $timestamp ) . + ' OR wl_notificationtimestamp IS NULL'; + } + // New notifications since the reset should not be cleared + $casTimeCond = 'wl_notificationtimestamp < ' . + $dbw->addQuotes( $dbw->timestamp( $this->params['casTime'] ) ) . + ' OR wl_notificationtimestamp IS NULL'; - $asOfTimes = array_unique( $dbw->selectFieldValues( - 'watchlist', - 'wl_notificationtimestamp', - [ 'wl_user' => $this->params['userId'], 'wl_notificationtimestamp IS NOT NULL' ], - __METHOD__, - [ 'ORDER BY' => 'wl_notificationtimestamp DESC' ] - ) ); - - foreach ( array_chunk( $asOfTimes, $rowsPerQuery ) as $asOfTimeBatch ) { - $dbw->update( + $firstBatch = true; + do { + $idsToUpdate = $dbw->selectFieldValues( 'watchlist', - [ 'wl_notificationtimestamp' => null ], + 'wl_id', [ 'wl_user' => $this->params['userId'], - 'wl_notificationtimestamp' => $asOfTimeBatch, - // New notifications since the reset should not be cleared - 'wl_notificationtimestamp < ' . - $dbw->addQuotes( $dbw->timestamp( $this->params['casTime'] ) ) + $timestampCond, + $casTimeCond, ], - __METHOD__ + __METHOD__, + [ 'LIMIT' => $rowsPerQuery ] ); - $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket ); - } + if ( $idsToUpdate ) { + $dbw->update( + 'watchlist', + [ 'wl_notificationtimestamp' => $timestamp ], + [ + 'wl_id' => $idsToUpdate, + // For paranoia, enforce the CAS time condition here too + $casTimeCond + ], + __METHOD__ + ); + if ( !$firstBatch ) { + $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket ); + } + $firstBatch = false; + } + } while ( $idsToUpdate ); } } diff --git a/includes/watcheditem/WatchedItemStore.php b/includes/watcheditem/WatchedItemStore.php index e092859089..8aca689a24 100644 --- a/includes/watcheditem/WatchedItemStore.php +++ b/includes/watcheditem/WatchedItemStore.php @@ -211,6 +211,10 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac } } } + + $pageSeenKey = $this->getPageSeenTimestampsKey( $user ); + $this->latestUpdateCache->delete( $pageSeenKey ); + $this->stash->delete( $pageSeenKey ); } /** @@ -805,36 +809,64 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac } /** + * Set the "last viewed" timestamps for certain titles on a user's watchlist. + * + * If the $targets parameter is omitted or set to [], this method simply wraps + * resetAllNotificationTimestampsForUser(), and in that case you should instead call that method + * directly; support for omitting $targets is for backwards compatibility. + * + * If $targets is omitted or set to [], timestamps will be updated for every title on the user's + * watchlist, and this will be done through a DeferredUpdate. If $targets is a non-empty array, + * only the specified titles will be updated, and this will be done immediately (not deferred). + * * @since 1.27 * @param User $user - * @param string|int $timestamp - * @param LinkTarget[] $targets + * @param string|int $timestamp Value to set the "last viewed" timestamp to (null to clear) + * @param LinkTarget[] $targets Titles to set the timestamp for; [] means the entire watchlist * @return bool */ public function setNotificationTimestampsForUser( User $user, $timestamp, array $targets = [] ) { // Only loggedin user can have a watchlist - if ( $user->isAnon() ) { + if ( $user->isAnon() || $this->readOnlyMode->isReadOnly() ) { return false; } - $dbw = $this->getConnectionRef( DB_MASTER ); - - $conds = [ 'wl_user' => $user->getId() ]; - if ( $targets ) { - $batch = new LinkBatch( $targets ); - $conds[] = $batch->constructSet( 'wl', $dbw ); + if ( !$targets ) { + // Backwards compatibility + $this->resetAllNotificationTimestampsForUser( $user, $timestamp ); + return true; } + $rows = $this->getTitleDbKeysGroupedByNamespace( $targets ); + + $dbw = $this->getConnectionRef( DB_MASTER ); if ( $timestamp !== null ) { $timestamp = $dbw->timestamp( $timestamp ); } + $ticket = $this->lbFactory->getEmptyTransactionTicket( __METHOD__ ); + $affectedSinceWait = 0; - $dbw->update( - 'watchlist', - [ 'wl_notificationtimestamp' => $timestamp ], - $conds, - __METHOD__ - ); + // Batch update items per namespace + foreach ( $rows as $namespace => $namespaceTitles ) { + $rowBatches = array_chunk( $namespaceTitles, $this->updateRowsPerQuery ); + foreach ( $rowBatches as $toUpdate ) { + $dbw->update( + 'watchlist', + [ 'wl_notificationtimestamp' => $timestamp ], + [ + 'wl_user' => $user->getId(), + 'wl_namespace' => $namespace, + 'wl_title' => $toUpdate + ] + ); + $affectedSinceWait += $dbw->affectedRows(); + // Wait for replication every time we've touched updateRowsPerQuery rows + if ( $affectedSinceWait >= $this->updateRowsPerQuery ) { + $this->lbFactory->commitAndWaitForReplication( __METHOD__, $ticket ); + $affectedSinceWait = 0; + } + } + } $this->uncacheUser( $user ); @@ -859,7 +891,13 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac return $timestamp; } - public function resetAllNotificationTimestampsForUser( User $user ) { + /** + * Schedule a DeferredUpdate that sets all of the "last viewed" timestamps for a given user + * to the same value. + * @param User $user + * @param string|int|null $timestamp Value to set all timestamps to, null to clear them + */ + public function resetAllNotificationTimestampsForUser( User $user, $timestamp = null ) { // Only loggedin user can have a watchlist if ( $user->isAnon() ) { return; @@ -868,7 +906,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac // If the page is watched by the user (or may be watched), update the timestamp $job = new ClearWatchlistNotificationsJob( $user->getUserPage(), - [ 'userId' => $user->getId(), 'casTime' => time() ] + [ 'userId' => $user->getId(), 'timestamp' => $timestamp, 'casTime' => time() ] ); // Try to run this post-send @@ -1191,7 +1229,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac } /** - * @param TitleValue[] $titles + * @param LinkTarget[] $titles * @return array */ private function getTitleDbKeysGroupedByNamespace( array $titles ) { diff --git a/tests/phpunit/includes/watcheditem/WatchedItemStoreIntegrationTest.php b/tests/phpunit/includes/watcheditem/WatchedItemStoreIntegrationTest.php index 6a383a2211..20dbedb50b 100644 --- a/tests/phpunit/includes/watcheditem/WatchedItemStoreIntegrationTest.php +++ b/tests/phpunit/includes/watcheditem/WatchedItemStoreIntegrationTest.php @@ -1,6 +1,7 @@ assertTrue( - $store->setNotificationTimestampsForUser( $user, '20200202020202', [ $title ] ) + $store->setNotificationTimestampsForUser( $user, '20100202020202', [ $title ] ) ); $this->assertEquals( - '20200202020202', + '20100202020202', $store->getWatchedItem( $user, $title )->getNotificationTimestamp() ); // setNotificationTimestampsForUser not specifying a title + // This will try to use a DeferredUpdate; disable that + $mockCallback = function ( $callback ) { + $callback(); + }; + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); $this->assertTrue( - $store->setNotificationTimestampsForUser( $user, '20210202020202' ) + $store->setNotificationTimestampsForUser( $user, '20110202020202' ) ); + // Because the operation above is normally deferred, it doesn't clear the cache + // Clear the cache manually + $wrappedStore = TestingAccessWrapper::newFromObject( $store ); + $wrappedStore->uncacheUser( $user ); $this->assertEquals( - '20210202020202', + '20110202020202', $store->getWatchedItem( $user, $title )->getNotificationTimestamp() ); } diff --git a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php index 6249c49961..a6b2162962 100644 --- a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php @@ -120,6 +120,9 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { $mock->expects( $this->any() ) ->method( 'getId' ) ->will( $this->returnValue( $id ) ); + $mock->expects( $this->any() ) + ->method( 'getUserPage' ) + ->will( $this->returnValue( Title::makeTitle( NS_USER, 'MockUser' ) ) ); return $mock; } @@ -2628,59 +2631,46 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { $user = $this->getMockNonAnonUserWithId( 1 ); $timestamp = '20100101010101'; - $mockDb = $this->getMockDb(); - $mockDb->expects( $this->once() ) - ->method( 'update' ) - ->with( - 'watchlist', - [ 'wl_notificationtimestamp' => 'TS' . $timestamp . 'TS' ], - [ 'wl_user' => 1 ] - ) - ->will( $this->returnValue( true ) ); - $mockDb->expects( $this->exactly( 1 ) ) - ->method( 'timestamp' ) - ->will( $this->returnCallback( function ( $value ) { - return 'TS' . $value . 'TS'; - } ) ); - $store = $this->newWatchedItemStore( - $this->getMockLBFactory( $mockDb ), + $this->getMockLBFactory( $this->getMockDb() ), $this->getMockJobQueueGroup(), $this->getMockCache(), $this->getMockReadOnlyMode() ); + // Note: This does not actually assert the job is correct + $callableCallCounter = 0; + $mockCallback = function ( $callable ) use ( &$callableCallCounter ) { + $callableCallCounter++; + $this->assertInternalType( 'callable', $callable ); + }; + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); + $this->assertTrue( $store->setNotificationTimestampsForUser( $user, $timestamp ) ); + $this->assertEquals( 1, $callableCallCounter ); } public function testSetNotificationTimestampsForUser_nullTimestamp() { $user = $this->getMockNonAnonUserWithId( 1 ); $timestamp = null; - $mockDb = $this->getMockDb(); - $mockDb->expects( $this->once() ) - ->method( 'update' ) - ->with( - 'watchlist', - [ 'wl_notificationtimestamp' => null ], - [ 'wl_user' => 1 ] - ) - ->will( $this->returnValue( true ) ); - $mockDb->expects( $this->exactly( 0 ) ) - ->method( 'timestamp' ) - ->will( $this->returnCallback( function ( $value ) { - return 'TS' . $value . 'TS'; - } ) ); - $store = $this->newWatchedItemStore( - $this->getMockLBFactory( $mockDb ), + $this->getMockLBFactory( $this->getMockDb() ), $this->getMockJobQueueGroup(), $this->getMockCache(), $this->getMockReadOnlyMode() ); + // Note: This does not actually assert the job is correct + $callableCallCounter = 0; + $mockCallback = function ( $callable ) use ( &$callableCallCounter ) { + $callableCallCounter++; + $this->assertInternalType( 'callable', $callable ); + }; + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); + $this->assertTrue( $store->setNotificationTimestampsForUser( $user, $timestamp ) ); @@ -2697,7 +2687,7 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { ->with( 'watchlist', [ 'wl_notificationtimestamp' => 'TS' . $timestamp . 'TS' ], - [ 'wl_user' => 1, 0 => 'makeWhereFrom2d return value' ] + [ 'wl_user' => 1, 'wl_namespace' => 0, 'wl_title' => [ 'Foo', 'Bar' ] ] ) ->will( $this->returnValue( true ) ); $mockDb->expects( $this->exactly( 1 ) ) @@ -2706,13 +2696,8 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { return 'TS' . $value . 'TS'; } ) ); $mockDb->expects( $this->once() ) - ->method( 'makeWhereFrom2d' ) - ->with( - [ [ 'Foo' => 1, 'Bar' => 1 ] ], - $this->isType( 'string' ), - $this->isType( 'string' ) - ) - ->will( $this->returnValue( 'makeWhereFrom2d return value' ) ); + ->method( 'affectedRows' ) + ->will( $this->returnValue( 2 ) ); $store = $this->newWatchedItemStore( $this->getMockLBFactory( $mockDb ), -- 2.20.1