Skip to content

Commit ebe5e39

Browse files
artongeartonge
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>
1 parent cddb171 commit ebe5e39

File tree

3 files changed

+39
-41
lines changed

3 files changed

+39
-41
lines changed

‎apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php‎

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

10+
use OC\Files\SetupManager;
1011
use OC\Files\View;
1112
use OCA\Files_Trashbin\Expiration;
1213
use OCA\Files_Trashbin\Helper;
1314
use OCA\Files_Trashbin\Trashbin;
1415
use OCP\AppFramework\Utility\ITimeFactory;
1516
use OCP\BackgroundJob\TimedJob;
1617
use OCP\IAppConfig;
18+
use OCP\IUser;
1719
use OCP\IUserManager;
1820
use Psr\Log\LoggerInterface;
1921

2022
class ExpireTrash extends TimedJob {
23+
private const THIRTY_MINUTES = 30 * 60;
24+
private const BATCH_SIZE = 10;
25+
2126
public function __construct(
2227
private IAppConfig $appConfig,
2328
private IUserManager $userManager,
2429
private Expiration $expiration,
2530
private LoggerInterface $logger,
31+
private SetupManager $setupManager,
2632
ITimeFactory $time,
2733
) {
2834
parent::__construct($time);
29-
// Run once per 30 minutes
30-
$this->setInterval(60 * 30);
35+
$this->setInterval(self::THIRTY_MINUTES);
3136
}
3237

3338
protected function run($argument) {
@@ -41,44 +46,47 @@ protected function run($argument) {
4146
return;
4247
}
4348

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);
49+
$stopTime = time() + self::THIRTY_MINUTES;
50+
51+
do {
52+
$this->appConfig->clearCache();
53+
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
54+
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset + self::BATCH_SIZE);
55+
56+
$users = $this->userManager->getSeenUsers($offset, self::BATCH_SIZE);
57+
$count = 0;
4758

48-
foreach ($users as $user) {
49-
try {
59+
foreach ($users as $user) {
5060
$uid = $user->getUID();
51-
if (!$this->setupFS($uid)) {
52-
continue;
61+
$count++;
62+
63+
try {
64+
if ($this->setupFS($user)) {
65+
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
66+
Trashbin::deleteExpiredFiles($dirContent, $uid);
67+
}
68+
} catch (\Throwable $e) {
69+
$this->logger->error('Error while expiring trashbin for user ' . $uid, ['exception' => $e]);
70+
} finally {
71+
$this->setupManager->tearDown();
5372
}
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]);
5873
}
5974

60-
$offset++;
75+
} while (time() < $stopTime && $count === self::BATCH_SIZE);
6176

62-
if ($stopTime < time()) {
63-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
64-
\OC_Util::tearDownFS();
65-
return;
66-
}
77+
if ($count < self::BATCH_SIZE) {
78+
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
6779
}
68-
69-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
70-
\OC_Util::tearDownFS();
7180
}
7281

7382
/**
7483
* Act on behalf on trash item owner
7584
*/
76-
protected function setupFS(string $user): bool {
77-
\OC_Util::tearDownFS();
78-
\OC_Util::setupFS($user);
85+
protected function setupFS(IUser $user): bool {
86+
$this->setupManager->setupForUser($user);
7987

8088
// Check if this user has a trashbin directory
81-
$view = new View('/' . $user);
89+
$view = new View('/' . $user->getUID());
8290
if (!$view->is_dir('/files_trashbin/files')) {
8391
return false;
8492
}

‎apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
1111

12+
use OC\Files\SetupManager;
1213
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1314
use OCA\Files_Trashbin\Expiration;
1415
use OCP\AppFramework\Utility\ITimeFactory;
@@ -26,6 +27,7 @@ class ExpireTrashTest extends TestCase {
2627
private IJobList&MockObject $jobList;
2728
private LoggerInterface&MockObject $logger;
2829
private ITimeFactory&MockObject $time;
30+
private SetupManager&MockObject $setupManager;
2931

3032
protected function setUp(): void {
3133
parent::setUp();
@@ -35,6 +37,7 @@ protected function setUp(): void {
3537
$this->expiration = $this->createMock(Expiration::class);
3638
$this->jobList = $this->createMock(IJobList::class);
3739
$this->logger = $this->createMock(LoggerInterface::class);
40+
$this->setupManager = $this->createMock(SetupManager::class);
3841

3942
$this->time = $this->createMock(ITimeFactory::class);
4043
$this->time->method('getTime')
@@ -54,7 +57,7 @@ public function testConstructAndRun(): void {
5457
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
5558
->willReturn(0);
5659

57-
$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);
5861
$job->start($this->jobList);
5962
}
6063

@@ -65,7 +68,7 @@ public function testBackgroundJobDeactivated(): void {
6568
$this->expiration->expects($this->never())
6669
->method('getMaxAgeAsTimestamp');
6770

68-
$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);
6972
$job->start($this->jobList);
7073
}
7174
}

‎build/psalm-baseline.xml‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1776,19 +1776,6 @@
17761776
<code><![CDATA[moveMount]]></code>
17771777
</UndefinedMethod>
17781778
</file>
1779-
<file src="apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php">
1780-
<DeprecatedClass>
1781-
<code><![CDATA[\OC_Util::setupFS($user)]]></code>
1782-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1783-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1784-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1785-
</DeprecatedClass>
1786-
<DeprecatedMethod>
1787-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1788-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1789-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1790-
</DeprecatedMethod>
1791-
</file>
17921779
<file src="apps/files_trashbin/lib/Command/CleanUp.php">
17931780
<DeprecatedClass>
17941781
<code><![CDATA[\OC_Util::setupFS($uid)]]></code>

0 commit comments

Comments
 (0)