Hoo man has uploaded a new change for review.
https://gerrit.wikimedia.org/r/181258
Change subject: Make UpdateRepo more robust
......................................................................
Make UpdateRepo more robust
Jobs running within 1s of the actual action were causing problems,
as due to replag etc. the API might not be aware of the changes.
To avoid that, we delay these jobs for 10s now (if possible).
Also we only normalize page names for move once now.
Change-Id: Ia5c3e112c6b0ed9b1f8cc0f10dc35ecf688760ba
---
M client/includes/UpdateRepo/UpdateRepo.php
M client/includes/UpdateRepo/UpdateRepoOnDelete.php
M client/includes/UpdateRepo/UpdateRepoOnMove.php
M client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
M client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
M repo/includes/UpdateRepo/UpdateRepoJob.php
M repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
7 files changed, 132 insertions(+), 36 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/58/181258/1
diff --git a/client/includes/UpdateRepo/UpdateRepo.php
b/client/includes/UpdateRepo/UpdateRepo.php
index 4480f1a..13e2758 100644
--- a/client/includes/UpdateRepo/UpdateRepo.php
+++ b/client/includes/UpdateRepo/UpdateRepo.php
@@ -134,7 +134,7 @@
public function injectJob( JobQueueGroup $jobQueueGroup ) {
wfProfileIn( __METHOD__ );
- $job = $this->createJob();
+ $job = $this->createJob( $jobQueueGroup );
wfProfileIn( __METHOD__ . '#push' );
$jobQueueGroup->push( $job );
@@ -146,19 +146,35 @@
/**
* Returns a new job for updating the repo.
*
+ * @param JobQueueGroup $jobQueueGroup
+ *
* @return IJobSpecification
*/
- public function createJob() {
+ public function createJob( JobQueueGroup $jobQueueGroup ) {
wfProfileIn( __METHOD__ );
+
+ $params = $this->getJobParameters();
+ if ( $this->delayJobs( $jobQueueGroup ) ) {
+ $params['jobReleaseTimestamp'] = time() +
$this->getJobDelay();
+ }
$job = new JobSpecification(
$this->getJobName(),
- $this->getJobParameters()
+ $params
);
wfProfileOut( __METHOD__ );
return $job;
+ }
+
+ /**
+ * @param JobQueueGroup $jobQueueGroup
+ *
+ * @return bool
+ */
+ private function delayJobs( JobQueueGroup $jobQueueGroup ) {
+ return $jobQueueGroup->get( $this->getJobName()
)->delayedJobsEnabled();
}
/**
@@ -175,4 +191,12 @@
*/
abstract protected function getJobName();
+ /**
+ * Get the time (in seconds) for which the job execution should be
delayed
+ * (if delayed jobs are enabled).
+ *
+ * @return int
+ */
+ abstract protected function getJobDelay();
+
}
diff --git a/client/includes/UpdateRepo/UpdateRepoOnDelete.php
b/client/includes/UpdateRepo/UpdateRepoOnDelete.php
index 84ae7b5..cfb4873 100644
--- a/client/includes/UpdateRepo/UpdateRepoOnDelete.php
+++ b/client/includes/UpdateRepo/UpdateRepoOnDelete.php
@@ -34,4 +34,16 @@
'user' => $this->user->getName()
);
}
+
+ /**
+ * Get the time (in seconds) for which the job execution should be
delayed
+ * (if delayed jobs are enabled).
+ *
+ * @return int
+ */
+ protected function getJobDelay() {
+ // Make sure this is not being run in the next 10s, as
otherwise the job
+ // might run before the client's api is up with what happened
(replag)
+ return 10;
+ }
}
diff --git a/client/includes/UpdateRepo/UpdateRepoOnMove.php
b/client/includes/UpdateRepo/UpdateRepoOnMove.php
index 272ca41..d5dcaa0 100644
--- a/client/includes/UpdateRepo/UpdateRepoOnMove.php
+++ b/client/includes/UpdateRepo/UpdateRepoOnMove.php
@@ -64,4 +64,15 @@
);
}
+ /**
+ * Get the time (in seconds) for which the job execution should be
delayed
+ * (if delayed jobs are enabled).
+ *
+ * @return int
+ */
+ protected function getJobDelay() {
+ // Make sure this is not being run in the next 10s, as
otherwise the job
+ // might run before the client's api is up with what happened
(replag)
+ return 10;
+ }
}
diff --git
a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
index b566eb5..b5c2248 100644
--- a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
+++ b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
@@ -4,6 +4,7 @@
use Title;
use User;
+use JobSpecification;
use Wikibase\Client\UpdateRepo\UpdateRepoOnDelete;
use Wikibase\DataModel\Entity\ItemId;
@@ -68,20 +69,32 @@
/**
* Get a JobQueueGroup mock for the use in UpdateRepo::injectJob.
*
- * @param \Job $expectedJob The job that is expected to be pushed
- * @param bool $success Whether the push will succeed
- *
* @return object
*/
- protected function getJobQueueGroupMock( $expectedJob, $success ) {
+ protected function getJobQueueGroupMock() {
$jobQueueGroupMock = $this->getMockBuilder( '\JobQueueGroup' )
->disableOriginalConstructor()
->getMock();
+ $self = $this; // PHP 5.3 compat
$jobQueueGroupMock->expects( $this->once() )
->method( 'push' )
- ->will( $this->returnValue( $success ) )
- ->with( $this->equalTo( $expectedJob ) );
+ ->will(
+ $this->returnCallback( function(
JobSpecification $job ) use( $self ) {
+ $self->verifyJob( $job );
+ } )
+ );
+
+ // Use JobQueueRedis over here, as mocking abstract classes
sucks
+ // and it doesn't matter anyway
+ $jobQueue = $this->getMockBuilder( '\JobQueueRedis' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $jobQueueGroupMock->expects( $this->once() )
+ ->method( 'get' )
+ ->with( $this->equalTo( 'UpdateRepoOnDelete' ) )
+ ->will( $this->returnValue( $jobQueue ) );
return $jobQueueGroupMock;
}
@@ -93,11 +106,11 @@
}
/**
- * Create a new job and verify the set params
+ * Verify a created job
+ *
+ * @param Job $job
*/
- public function testCreateJob() {
- $updateRepo = $this->getNewUpdateRepoOnDelete();
- $job = $updateRepo->createJob();
+ public function verifyJob( JobSpecification $job ) {
$itemId = new ItemId( 'Q123' );
$data = $this->getFakeData();
@@ -113,9 +126,8 @@
public function testInjectJob() {
$updateRepo = $this->getNewUpdateRepoOnDelete();
- $job = $updateRepo->createJob();
- $jobQueueGroupMock = $this->getJobQueueGroupMock( $job, true );
+ $jobQueueGroupMock = $this->getJobQueueGroupMock( true );
$updateRepo->injectJob( $jobQueueGroupMock );
}
diff --git a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
index 7d03bee..d021553 100644
--- a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
+++ b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
@@ -2,6 +2,7 @@
namespace Wikibase\Client\Tests\UpdateRepo;
+use JobSpecification;
use Wikibase\Client\UpdateRepo\UpdateRepoOnMove;
use Wikibase\DataModel\Entity\ItemId;
@@ -70,20 +71,36 @@
/**
* Get a JobQueueGroup mock for the use in UpdateRepo::injectJob.
*
- * @param \Job $expectedJob The job that is expected to be pushed
- * @param bool $success Whether the push will succeed
- *
* @return object
*/
- private function getJobQueueGroupMock( $expectedJob, $success ) {
+ protected function getJobQueueGroupMock() {
$jobQueueGroupMock = $this->getMockBuilder( '\JobQueueGroup' )
->disableOriginalConstructor()
->getMock();
+ $self = $this; // PHP 5.3 compat
$jobQueueGroupMock->expects( $this->once() )
->method( 'push' )
- ->will( $this->returnValue( $success ) )
- ->with( $this->equalTo( $expectedJob ) );
+ ->will(
+ $this->returnCallback( function(
JobSpecification $job ) use( $self ) {
+ $self->verifyJob( $job );
+ } )
+ );
+
+ // Use JobQueueRedis over here, as mocking abstract classes
sucks
+ // and it doesn't matter anyway
+ $jobQueue = $this->getMockBuilder( '\JobQueueRedis' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $jobQueue->expects( $this->any() )
+ ->method( 'delayedJobsEnabled' )
+ ->will( $this->returnValue( true ) );
+
+ $jobQueueGroupMock->expects( $this->once() )
+ ->method( 'get' )
+ ->with( $this->equalTo( 'UpdateRepoOnMove' ) )
+ ->will( $this->returnValue( $jobQueue ) );
return $jobQueueGroupMock;
}
@@ -95,11 +112,11 @@
}
/**
- * Create a new job and verify the set params
+ * Verify a created job
+ *
+ * @param Job $job
*/
- public function testCreateJob() {
- $updateRepo = $this->getNewLocal();
- $job = $updateRepo->createJob();
+ public function verifyJob( JobSpecification $job ) {
$itemId = new ItemId( 'Q123' );
$moveData = $this->getFakeMoveData();
@@ -116,9 +133,8 @@
public function testInjectJob() {
$updateRepo = $this->getNewLocal();
- $job = $updateRepo->createJob();
- $jobQueueGroupMock = $this->getJobQueueGroupMock( $job, true );
+ $jobQueueGroupMock = $this->getJobQueueGroupMock( true );
$updateRepo->injectJob( $jobQueueGroupMock );
}
diff --git a/repo/includes/UpdateRepo/UpdateRepoJob.php
b/repo/includes/UpdateRepo/UpdateRepoJob.php
index 92a3f8b..4149ed7 100644
--- a/repo/includes/UpdateRepo/UpdateRepoJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoJob.php
@@ -177,7 +177,7 @@
);
if ( !$status->isOK() ) {
- wfDebugLog( __CLASS__, __FUNCTION__ . ": attemptSave
failed: " . $status->getMessage()->text() );
+ wfDebugLog( __CLASS__, __FUNCTION__ . ": attemptSave
for " . $item->getId()->getSerialization() . " failed: " .
$status->getMessage()->text() );
}
wfProfileOut( __METHOD__ );
diff --git a/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
b/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
index f479eb2..b9aa91f 100644
--- a/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
@@ -32,6 +32,11 @@
private $siteStore;
/**
+ * @var string|bool|null
+ */
+ private $normalizedPageName = null;
+
+ /**
* Constructs a UpdateRepoOnMoveJob propagating a page move to the repo
*
* @note: This is for use by Job::factory, don't call it directly;
@@ -116,6 +121,27 @@
}
/**
+ * @return string|bool False in case the normalization failed
+ */
+ private function getNormalizedPageName() {
+ if ( $this->normalizedPageName === null ) {
+ $params = $this->getParams();
+ $newPage = $params['newTitle'];
+ $siteId = $params['siteId'];
+
+ $site = $this->siteStore->getSite( $siteId );
+ $this->normalizedPageName = $site->normalizePageName(
$newPage );
+
+ if ( $this->normalizedPageName === false ) {
+ wfDebugLog( 'UpdateRepo', "OnMove: Normalizing
the page name $newPage on $siteId failed" );
+ }
+
+ }
+
+ return $this->normalizedPageName;
+ }
+
+ /**
* Whether the propagated update is valid (and thus should be applied)
*
* @param Item $item
@@ -127,7 +153,6 @@
$params = $this->getParams();
$siteId = $params['siteId'];
$oldPage = $params['oldTitle'];
- $newPage = $params['newTitle'];
$oldSiteLink = $this->getSiteLink( $item, $siteId );
if ( !$oldSiteLink || $oldSiteLink->getPageName() !== $oldPage
) {
@@ -137,10 +162,8 @@
return false;
}
- $site = $this->siteStore->getSite( $siteId );
- // Normalize the name again, just in case the page has been
updated in the mean time
- if ( !$site || !$site->normalizePageName( $newPage ) ) {
- wfDebugLog( 'UpdateRepo', "OnMove: Normalizing the page
name $newPage on $siteId failed" );
+ // Normalize the name, just in case the page has been updated
in the mean time
+ if ( $this->getNormalizedPageName() === false ) {
wfProfileOut( __METHOD__ );
return false;
}
@@ -158,14 +181,12 @@
protected function applyChanges( Item $item ) {
$params = $this->getParams();
$siteId = $params['siteId'];
- $newPage = $params['newTitle'];
$oldSiteLink = $this->getSiteLink( $item, $siteId );
- $site = $this->siteStore->getSite( $siteId );
$siteLink = new SiteLink(
$siteId,
- $site->normalizePageName( $newPage ),
+ $this->getNormalizedPageName(),
$oldSiteLink->getBadges() // Keep badges
);
--
To view, visit https://gerrit.wikimedia.org/r/181258
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia5c3e112c6b0ed9b1f8cc0f10dc35ecf688760ba
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Hoo man <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
