WatchedItemStore: Use batching in setNotificationTimestampsForUser
authorRoan Kattouw <roan.kattouw@gmail.com>
Wed, 16 Jan 2019 21:51:54 +0000 (13:51 -0800)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 21 Mar 2019 04:41:42 +0000 (04:41 +0000)
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
includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php
includes/watcheditem/WatchedItemStore.php
tests/phpunit/includes/watcheditem/WatchedItemStoreIntegrationTest.php
tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php

index c9ebfa8..ba4c6e8 100644 (file)
@@ -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 )
                                ? ''
index 94c1351..b71580a 100644 (file)
 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 );
        }
 }
index e092859..8aca689 100644 (file)
@@ -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 ) {
index 6a383a2..20dbedb 100644 (file)
@@ -1,6 +1,7 @@
 <?php
 
 use MediaWiki\MediaWikiServices;
+use Wikimedia\TestingAccessWrapper;
 
 /**
  * @author Addshore
@@ -199,19 +200,28 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase {
 
                // setNotificationTimestampsForUser specifying a title
                $this->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()
                );
        }
index 6249c49..a6b2162 100644 (file)
@@ -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 ),