Skip to content

Commit 75b1cd1

Browse files
committed
feat(files_trashbin): Refactor expire background job to support parallel run
- Follow-up of #51600 The original PR introduced the possibility to continue an `ExpireTrash` job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed. But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster. This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit. Signed-off-by: Louis Chemineau <louis@chmn.me> [skip ci] Signed-off-by: Louis Chemineau <louis@chmn.me>
1 parent b1a47eb commit 75b1cd1

File tree

2 files changed

+39
-45
lines changed

2 files changed

+39
-45
lines changed

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88
namespace OCA\Files_Trashbin\BackgroundJob;
99

10+
use OC\Files\SetupManager;
1011
use OCA\Files_Trashbin\Expiration;
1112
use OCA\Files_Trashbin\Helper;
1213
use OCA\Files_Trashbin\Trashbin;
@@ -17,17 +18,18 @@
1718
use Psr\Log\LoggerInterface;
1819

1920
class ExpireTrash extends TimedJob {
21+
private const THIRTY_MINUTES = 30 * 60;
2022

2123
public function __construct(
2224
private IAppConfig $appConfig,
2325
private IUserManager $userManager,
2426
private Expiration $expiration,
2527
private LoggerInterface $logger,
28+
private SetupManager $setupManager,
2629
ITimeFactory $time
2730
) {
2831
parent::__construct($time);
29-
// Run once per 30 minutes
30-
$this->setInterval(60 * 30);
32+
$this->setInterval(self::THIRTY_MINUTES);
3133
}
3234

3335
protected function run($argument) {
@@ -41,44 +43,43 @@ protected function run($argument) {
4143
return;
4244
}
4345

44-
$stopTime = time() + 60 * 30; // Stops after 30 minutes.
45-
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
46-
$users = $this->userManager->getSeenUsers($offset);
46+
$stopTime = time() + self::THIRTY_MINUTES;
4747

48-
foreach ($users as $user) {
49-
try {
50-
$uid = $user->getUID();
51-
if (!$this->setupFS($uid)) {
52-
continue;
48+
do {
49+
$this->appConfig->clearCache();
50+
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
51+
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset + 10);
52+
53+
$users = $this->userManager->getLastLoggedInUsers(10, $offset);
54+
55+
foreach ($users as $uid) {
56+
try {
57+
if ($this->setupFS($uid)) {
58+
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
59+
Trashbin::deleteExpiredFiles($dirContent, $uid);
60+
}
61+
} catch (\Throwable $e) {
62+
$this->logger->error('Error while expiring trashbin for user ' . $uid, ['exception' => $e]);
5363
}
54-
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
55-
Trashbin::deleteExpiredFiles($dirContent, $uid);
56-
} catch (\Throwable $e) {
57-
$this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]);
64+
65+
$this->setupManager->tearDown();
5866
}
5967

60-
$offset++;
68+
} while (time() < $stopTime && count($users) === 10);
6169

62-
if ($stopTime < time()) {
63-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
64-
\OC_Util::tearDownFS();
65-
return;
66-
}
70+
if (count($users) < 10) {
71+
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
6772
}
68-
69-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
70-
\OC_Util::tearDownFS();
7173
}
7274

7375
/**
7476
* Act on behalf on trash item owner
7577
*/
76-
protected function setupFS(string $user): bool {
77-
\OC_Util::tearDownFS();
78-
\OC_Util::setupFS($user);
78+
protected function setupFS(string $uid): bool {
79+
$this->setupManager->setupForUser($this->userManager->get($uid));
7980

8081
// Check if this user has a trashbin directory
81-
$view = new \OC\Files\View('/' . $user);
82+
$view = new View('/' . $uid);
8283
if (!$view->is_dir('/files_trashbin/files')) {
8384
return false;
8485
}

apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
1010

11+
use OC\Files\SetupManager;
1112
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1213
use OCA\Files_Trashbin\Expiration;
1314
use OCP\AppFramework\Utility\ITimeFactory;
@@ -19,23 +20,14 @@
1920
use Test\TestCase;
2021

2122
class ExpireTrashTest extends TestCase {
22-
/** @var IAppConfig&MockObject */
23-
private $appConfig;
2423

25-
/** @var IUserManager&MockObject */
26-
private $userManager;
27-
28-
/** @var Expiration&MockObject */
29-
private $expiration;
30-
31-
/** @var IJobList&MockObject */
32-
private $jobList;
33-
34-
/** @var LoggerInterface&MockObject */
35-
private $logger;
36-
37-
/** @var ITimeFactory&MockObject */
38-
private $time;
24+
private IAppConfig&MockObject $appConfig;
25+
private IUserManager&MockObject $userManager;
26+
private Expiration&MockObject $expiration;
27+
private IJobList&MockObject $jobList;
28+
private LoggerInterface&MockObject $logger;
29+
private ITimeFactory&MockObject $time;
30+
private SetupManager&MockObject $setupManager;
3931

4032
protected function setUp(): void {
4133
parent::setUp();
@@ -45,6 +37,7 @@ protected function setUp(): void {
4537
$this->expiration = $this->createMock(Expiration::class);
4638
$this->jobList = $this->createMock(IJobList::class);
4739
$this->logger = $this->createMock(LoggerInterface::class);
40+
$this->setupManager = $this->createMock(SetupManager::class);
4841

4942
$this->time = $this->createMock(ITimeFactory::class);
5043
$this->time->method('getTime')
@@ -64,7 +57,7 @@ public function testConstructAndRun(): void {
6457
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
6558
->willReturn(0);
6659

67-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
60+
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->setupManager, $this->time);
6861
$job->start($this->jobList);
6962
}
7063

@@ -75,7 +68,7 @@ public function testBackgroundJobDeactivated(): void {
7568
$this->expiration->expects($this->never())
7669
->method('getMaxAgeAsTimestamp');
7770

78-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
71+
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->setupManager, $this->time);
7972
$job->start($this->jobList);
8073
}
8174
}

0 commit comments

Comments
 (0)