jenkins-bot has submitted this change and it was merged.
Change subject: Make UpdateRepo more robust
......................................................................
Make UpdateRepo more robust
Jobs running within a second of the actual action were causing problems,
as due to replag etc. the API might not be aware of the changes.
To avoid that delay these jobs for the highest replag of any pooled
database server + 1 second now.
Also we only normalize page names for moves once now, and I made
UpdateRepo::createJob private as it was only used for testing.
Bug: T76545
Change-Id: Ia5c3e112c6b0ed9b1f8cc0f10dc35ecf688760ba
---
M client/includes/UpdateRepo/UpdateRepo.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
6 files changed, 117 insertions(+), 37 deletions(-)
Approvals:
Adrian Lang: Looks good to me, approved
jenkins-bot: Verified
diff --git a/client/includes/UpdateRepo/UpdateRepo.php
b/client/includes/UpdateRepo/UpdateRepo.php
index 4480f1a..0f0100e 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,14 +146,21 @@
/**
* Returns a new job for updating the repo.
*
+ * @param JobQueueGroup $jobQueueGroup
+ *
* @return IJobSpecification
*/
- public function createJob() {
+ private 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__ );
@@ -162,6 +169,31 @@
}
/**
+ * @param JobQueueGroup $jobQueueGroup
+ *
+ * @return bool
+ */
+ private function delayJobs( JobQueueGroup $jobQueueGroup ) {
+ return $jobQueueGroup->get( $this->getJobName()
)->delayedJobsEnabled();
+ }
+
+ /**
+ * Get the time (in seconds) for which the job execution should be
delayed
+ * (if delayed jobs are enabled). Defaults to the max replag of any
pooled
+ * DB server + 1 seconds.
+ *
+ * @return int
+ */
+ protected function getJobDelay() {
+ $lagArray = wfGetLB()->getMaxLag();
+ // This should be good enough, especially given that lagged
servers get
+ // less load by the load balancer, thus it's very unlikely
we'll end
+ // up on the server with the highest lag.
+ // Note: Always add at least +1 here, otherwise this can be -1
+ return $lagArray[1] + 1;
+ }
+
+ /**
* Get the parameters for creating a new IJobSpecification
*
* @return array
diff --git a/client/includes/UpdateRepo/UpdateRepoOnMove.php
b/client/includes/UpdateRepo/UpdateRepoOnMove.php
index 272ca41..319f249 100644
--- a/client/includes/UpdateRepo/UpdateRepoOnMove.php
+++ b/client/includes/UpdateRepo/UpdateRepoOnMove.php
@@ -63,5 +63,4 @@
'user' => $this->user->getName()
);
}
-
}
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 b7e3b52..37ecde1 100644
--- a/repo/includes/UpdateRepo/UpdateRepoJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoJob.php
@@ -179,7 +179,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 4f724ee..42b333f 100644
--- a/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoOnMoveJob.php
@@ -31,6 +31,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: merged
Gerrit-Change-Id: Ia5c3e112c6b0ed9b1f8cc0f10dc35ecf688760ba
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Hoo man <[email protected]>
Gerrit-Reviewer: Adrian Lang <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: JanZerebecki <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
