Skip to content

Commit c7f57a9

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 c7f57a9

File tree

3 files changed

+38
-40
lines changed

3 files changed

+38
-40
lines changed

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

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -7,27 +7,31 @@
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+
2125
public function __construct(
2226
private IAppConfig $appConfig,
2327
private IUserManager $userManager,
2428
private Expiration $expiration,
2529
private LoggerInterface $logger,
30+
private SetupManager $setupManager,
2631
ITimeFactory $time,
2732
) {
2833
parent::__construct($time);
29-
// Run once per 30 minutes
30-
$this->setInterval(60 * 30);
34+
$this->setInterval(self::THIRTY_MINUTES);
3135
}
3236

3337
protected function run($argument) {
@@ -41,44 +45,47 @@ protected function run($argument) {
4145
return;
4246
}
4347

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

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

60-
$offset++;
74+
} while (time() < $stopTime && $count === 10);
6175

62-
if ($stopTime < time()) {
63-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
64-
\OC_Util::tearDownFS();
65-
return;
66-
}
76+
if ($count < 10) {
77+
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
6778
}
68-
69-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
70-
\OC_Util::tearDownFS();
7179
}
7280

7381
/**
7482
* Act on behalf on trash item owner
7583
*/
76-
protected function setupFS(string $user): bool {
77-
\OC_Util::tearDownFS();
78-
\OC_Util::setupFS($user);
84+
protected function setupFS(IUser $user): bool {
85+
$this->setupManager->setupForUser($user);
7986

8087
// Check if this user has a trashbin directory
81-
$view = new View('/' . $user);
88+
$view = new View('/' . $user->getUID());
8289
if (!$view->is_dir('/files_trashbin/files')) {
8390
return false;
8491
}

‎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 & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1776,18 +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>
17911779
</file>
17921780
<file src="apps/files_trashbin/lib/Command/CleanUp.php">
17931781
<DeprecatedClass>

0 commit comments

Comments
 (0)