Lock jobs while executing them, to allow multiple executors to run in… by nickvergessen · Pull Request #24696 · owncloud/core

nickvergessen · 2016-05-18T12:33:27Z

… parallel

Contributes towards #24551

Clarify API
Fix tests

I implemented the UPDATE with lock and then SELECT because SELECT ... FOR UPDATE does not exist for all ours DBs, so it feels wrong adding that to the IQueryBuilder

nickvergessen · 2016-05-18T12:35:50Z

 	 * @param IJob $job
 	 */
 	public function setLastJob($job) {
+		$query = $this->connection->getQueryBuilder();


From API pov I'd expect this to happen in setLastRun(), but that is currently called before run(), so the job would be unlocked, although it is not finished yet:

core/lib/private/BackgroundJob/Job.php

Lines 50 to 52 in 6b5db08

$jobList->setLastRun($this);

try {

$this->run($this->argument);

But I guess that's part of #24609 😞

nickvergessen · 2016-05-18T13:52:57Z

cc @DeepDiver1975 that's what I ended up with atm. Feel free to comment or deny, however I don't see a way to get SELECT FOR UPDATE working

DeepDiver1975 · 2016-05-19T08:38:57Z

Select for Update is one way to Lock a row.
Each DBMS has a mechanism for this.
Worst case we might end up in locking the full table.

Without locking this will not work.

nickvergessen · 2016-05-19T13:07:01Z

Updated so it now locks the table for the select+update and then immediately unlocks it again, before building the job instance or anything else to keep the time window as small as possible.

nickvergessen · 2016-05-20T09:25:05Z

Test instructions

Create apps/files/lib/BackgroundJob/Test.php with content:

<?php

namespace OCA\Files\BackgroundJob;

class Test extends \OC\BackgroundJob\TimedJob {

public function __construct() {
    $this->setInterval(10);
}

protected function run($argument) {
    \OC::$server->getLogger()->error('PID ' . getmypid() . ' - ' . serialize($argument));
    sleep(rand(1, 3));
}
}

Drop jobs table content and add fake jobs:

INSERT INTO `oc_jobs` (`id`, `class`, `argument`, `last_run`, `last_checked`, `reserved_at`) VALUES
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '1', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '2', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '3', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '4', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '5', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '6', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '7', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '8', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '9', '0', '0', '0'),
(NULL, 'OCA\\Files\\BackgroundJob\\Test', '10', '0', '0', '0');

Open two console windows and run sudo -u www-data php cron.php in them.

Before:

Window 2:

sudo -u www-data php cron.php
Another instance of cron.php is still running!

Log:

{...,"message":"PID 12196 - i:1;","level":3,"time":"2016-05-20T09:05:53+00:00",...}
{...,"message":"PID 12196 - i:2;","level":3,"time":"2016-05-20T09:05:54+00:00",...}
{...,"message":"PID 12196 - i:4;","level":3,"time":"2016-05-20T09:05:57+00:00",...}
{...,"message":"PID 12196 - i:5;","level":3,"time":"2016-05-20T09:05:59+00:00",...}
{...,"message":"PID 12196 - i:6;","level":3,"time":"2016-05-20T09:06:00+00:00",...}
{...,"message":"PID 12196 - i:7;","level":3,"time":"2016-05-20T09:06:02+00:00",...}
{...,"message":"PID 12196 - i:8;","level":3,"time":"2016-05-20T09:06:04+00:00",...}
{...,"message":"PID 12196 - i:9;","level":3,"time":"2016-05-20T09:06:06+00:00",...}
{...,"message":"PID 12196 - i:10;","level":3,"time":"2016-05-20T09:06:09+00:00",...}
{...,"message":"PID 12196 - i:3;","level":3,"time":"2016-05-20T09:06:12+00:00",...}

After:

Both windows run
Log:

{...,"message":"PID 12554 - i:2;","level":3,"time":"2016-05-20T09:09:59+00:00",...}
{...,"message":"PID 12633 - i:4;","level":3,"time":"2016-05-20T09:10:01+00:00",...}
{...,"message":"PID 12554 - i:5;","level":3,"time":"2016-05-20T09:10:01+00:00",...}
{...,"message":"PID 12554 - i:6;","level":3,"time":"2016-05-20T09:10:02+00:00",...}
{...,"message":"PID 12633 - i:7;","level":3,"time":"2016-05-20T09:10:04+00:00",...}
{...,"message":"PID 12554 - i:8;","level":3,"time":"2016-05-20T09:10:04+00:00",...}
{...,"message":"PID 12633 - i:9;","level":3,"time":"2016-05-20T09:10:06+00:00",...}
{...,"message":"PID 12554 - i:10;","level":3,"time":"2016-05-20T09:10:06+00:00",...}
{...,"message":"PID 12554 - i:3;","level":3,"time":"2016-05-20T09:10:08+00:00",...}
{...,"message":"PID 12633 - i:1;","level":3,"time":"2016-05-20T09:10:09+00:00",...}

As you can see each job is still only executed once, but they are executed from different processes, so you can add more processes, when your job table grows too fast. Also the jobs are executed after ~10 seconds instead of ~20 so they are really executed in parallel.

@DeepDiver1975 @PVince81 ready to review from my pov

PVince81 · 2016-05-20T10:08:49Z

can you add a comment to explain what this column is about ?

PVince81 · 2016-05-20T10:30:12Z

Looks good, thanks for the latest changes 👍

PVince81 · 2016-05-20T10:38:28Z

Second review @icewind1991 @Xenopathic @DeepDiver1975 ?

icewind1991 · 2016-05-20T11:11:59Z

Looks good 👍

PVince81 · 2016-05-20T14:45:16Z

@nickvergessen I have some bad news for you.

Hint 1: it starts with an O
Hint 2: wakes up mostly when touching DB code
Hint 3: everybody hates fixing stuff for it

07:45:46 1) Test\BackgroundJob\JobList::testGetNext
07:45:46 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'LOCK TABLE "oc_jobs" EXCLUSIVE':
07:45:46 
07:45:46 ORA-01738: missing IN keyword
07:45:46 
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php:76
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:116
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:208
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/AdapterOCI8.php:35
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:321
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/BackgroundJob/JobList.php:191
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/tests/lib/backgroundjob/joblist.php:184
07:45:46 
07:45:46 Caused by
07:45:46 Doctrine\DBAL\Driver\OCI8\OCI8Exception: ORA-01738: missing IN keyword
07:45:46 
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Exception.php:33
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php:214
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php:140
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:993
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:208
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/AdapterOCI8.php:35
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:321
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/BackgroundJob/JobList.php:191
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/tests/lib/backgroundjob/joblist.php:184
07:45:46 
07:45:46 2) Test\BackgroundJob\JobList::testGetNextSkipReserved
07:45:46 BadMethodCallException: Can not lock a new table until the previous lock is released.
07:45:46 
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:316
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/BackgroundJob/JobList.php:191
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/tests/lib/backgroundjob/joblist.php:197
07:45:46 
07:45:46 3) Test\BackgroundJob\JobList::testGetNextSkipNonExisting
07:45:46 BadMethodCallException: Can not lock a new table until the previous lock is released.
07:45:46 
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/DB/Connection.php:316
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/lib/private/BackgroundJob/JobList.php:191
07:45:46 /ssd/jenkins/workspace/core-ci-linux@4/database/oci/label/SLAVE/tests/lib/backgroundjob/joblist.php:211
07:45:46

… parallel

nickvergessen · 2016-05-21T00:01:51Z

Easy fix, forgot one word in the LOCK TABLE x >>>IN<<< .... sql command, fixed

lock · 2019-08-05T20:02:10Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

nickvergessen added 3 - To Review technical debt p1-urgent Critical issue, need to consider hotfix with just that issue feature:backgroundjobs labels May 18, 2016

nickvergessen added this to the 9.1-current milestone May 18, 2016

nickvergessen reviewed May 18, 2016
View reviewed changes

nickvergessen force-pushed the lock-jobs-while-executing branch 3 times, most recently from 2f17089 to 98478ab Compare May 19, 2016 13:03

nickvergessen added 2 - Developing and removed 3 - To Review labels May 20, 2016

nickvergessen force-pushed the lock-jobs-while-executing branch 2 times, most recently from 19e3c7b to 8acb253 Compare May 20, 2016 09:17

PVince81 added 3 - To Review and removed 2 - Developing labels May 20, 2016

PVince81 reviewed May 20, 2016
View reviewed changes

Comment thread db_structure.xml Outdated

PVince81 May 20, 2016

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment to explain what this column is about ?

icewind1991 added 4 - To release and removed 3 - To Review labels May 20, 2016

PVince81 added the 2 - Developing label May 20, 2016

PVince81 removed the 4 - To release label May 20, 2016

nickvergessen added 3 commits May 21, 2016 01:59

Add a method to lock a table

7e3ce83

Lock jobs while executing them, to allow multiple executors to run in…

d0a2fa0

… parallel

Do not lock the cron anymore so we can have multiple workers

92c21fd

nickvergessen force-pushed the lock-jobs-while-executing branch from 9ec6ecc to 92c21fd Compare May 20, 2016 23:59

nickvergessen added 4 - To release and removed 2 - Developing labels May 21, 2016

PVince81 merged commit 8646802 into master May 21, 2016

PVince81 deleted the lock-jobs-while-executing branch May 21, 2016 17:08

lock Bot locked as resolved and limited conversation to collaborators Aug 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Lock jobs while executing them, to allow multiple executors to run in…#24696

Lock jobs while executing them, to allow multiple executors to run in…#24696
PVince81 merged 3 commits into
masterfrom
lock-jobs-while-executing

nickvergessen commented May 18, 2016 •

edited

Loading

Uh oh!

nickvergessen May 18, 2016

Uh oh!

nickvergessen commented May 18, 2016

Uh oh!

DeepDiver1975 commented May 19, 2016

Uh oh!

nickvergessen commented May 19, 2016

Uh oh!

nickvergessen commented May 20, 2016 •

edited

Loading

Uh oh!

PVince81 May 20, 2016

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

icewind1991 commented May 20, 2016

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

nickvergessen commented May 21, 2016

Uh oh!

lock Bot commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	$jobList->setLastRun($this);
	try {
	$this->run($this->argument);

Uh oh!

Conversation

nickvergessen commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickvergessen May 18, 2016

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented May 18, 2016

Uh oh!

DeepDiver1975 commented May 19, 2016

Uh oh!

nickvergessen commented May 19, 2016

Uh oh!

nickvergessen commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test instructions

Before:

After:

Uh oh!

PVince81 May 20, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

icewind1991 commented May 20, 2016

Uh oh!

PVince81 commented May 20, 2016

Uh oh!

nickvergessen commented May 21, 2016

Uh oh!

lock Bot commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickvergessen commented May 18, 2016 •

edited

Loading

nickvergessen commented May 20, 2016 •

edited

Loading