Lock jobs while executing them, to allow multiple executors to run in… #24696
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickvergessen
added
3 - To Review
technical debt
p1-urgent
| @@ -267,13 +289,23 @@ private function buildJob($row) { | |||
| * @param IJob $job | |||
| */ | |||
| public function setLastJob($job) { | |||
| $query = $this->connection->getQueryBuilder(); | |||
There was a problem hiding this comment.
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
But I guess that's part of #24609 😞
|
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 |
|
Select for Update is one way to Lock a row. Without locking this will not work. |
nickvergessen
force-pushed
the
lock-jobs-while-executing
branch
3 times, most recently
from
2f17089
to
98478ab
Compare
|
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
force-pushed
the
lock-jobs-while-executing
branch
2 times, most recently
from
19e3c7b
to
8acb253
Compare
Test instructions
Before:Window 2: 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 {...,"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 |
| </field> | ||
|
|
||
| <field> | ||
| <name>reserved_at</name> |
There was a problem hiding this comment.
can you add a comment to explain what this column is about ?
|
Looks good, thanks for the latest changes 👍 |
|
Second review @icewind1991 @Xenopathic @DeepDiver1975 ? |
|
Looks good 👍 |
|
@nickvergessen I have some bad news for you. Hint 1: it starts with an O |
nickvergessen
force-pushed
the
lock-jobs-while-executing
branch
from
9ec6ecc
to
92c21fd
Compare
|
Easy fix, forgot one word in the |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… parallel
Contributes towards #24551
I implemented the
UPDATE with lock and then SELECTbecauseSELECT ... FOR UPDATEdoes not exist for all ours DBs, so it feels wrong adding that to theIQueryBuilder