Skip to content

Commit ed57aa2

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 9fa0477 commit ed57aa2

File tree

3 files changed

+40
-58
lines changed

3 files changed

+40
-58
lines changed

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

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

9+
use OC\Files\SetupManager;
910
use OC\Files\View;
1011
use OCA\Files_Trashbin\Expiration;
1112
use OCA\Files_Trashbin\Helper;
@@ -17,16 +18,18 @@
1718
use Psr\Log\LoggerInterface;
1819

1920
class ExpireTrash extends TimedJob {
21+
private const THIRTY_MINUTES = 30 * 60;
22+
2023
public function __construct(
2124
private IAppConfig $appConfig,
2225
private IUserManager $userManager,
2326
private Expiration $expiration,
2427
private LoggerInterface $logger,
28+
private SetupManager $setupManager,
2529
ITimeFactory $time,
2630
) {
2731
parent::__construct($time);
28-
// Run once per 30 minutes
29-
$this->setInterval(60 * 30);
32+
$this->setInterval(self::THIRTY_MINUTES);
3033
}
3134

3235
protected function run($argument) {
@@ -40,44 +43,43 @@ protected function run($argument) {
4043
return;
4144
}
4245

43-
$stopTime = time() + 60 * 30; // Stops after 30 minutes.
44-
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
45-
$users = $this->userManager->getSeenUsers($offset);
46+
$stopTime = time() + self::THIRTY_MINUTES;
47+
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);
4654

47-
foreach ($users as $user) {
48-
try {
49-
$uid = $user->getUID();
50-
if (!$this->setupFS($uid)) {
51-
continue;
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]);
5263
}
53-
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
54-
Trashbin::deleteExpiredFiles($dirContent, $uid);
55-
} catch (\Throwable $e) {
56-
$this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]);
64+
65+
$this->setupManager->tearDown();
5766
}
5867

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

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

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

7981
// Check if this user has a trashbin directory
80-
$view = new View('/' . $user);
82+
$view = new View('/' . $uid);
8183
if (!$view->is_dir('/files_trashbin/files')) {
8284
return false;
8385
}

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

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

88
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
99

10+
use OC\Files\SetupManager;
1011
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1112
use OCA\Files_Trashbin\Expiration;
1213
use OCP\AppFramework\Utility\ITimeFactory;
@@ -18,23 +19,14 @@
1819
use Test\TestCase;
1920

2021
class ExpireTrashTest extends TestCase {
21-
/** @var IAppConfig&MockObject */
22-
private $appConfig;
2322

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

3931
protected function setUp(): void {
4032
parent::setUp();
@@ -44,6 +36,7 @@ protected function setUp(): void {
4436
$this->expiration = $this->createMock(Expiration::class);
4537
$this->jobList = $this->createMock(IJobList::class);
4638
$this->logger = $this->createMock(LoggerInterface::class);
39+
$this->setupManager = $this->createMock(SetupManager::class);
4740

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

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

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

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

‎build/psalm-baseline.xml‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1792,19 +1792,6 @@
17921792
<code><![CDATA[isset($_['hideFileList']) && $_['hideFileList'] !== true]]></code>
17931793
</RedundantCondition>
17941794
</file>
1795-
<file src="apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php">
1796-
<DeprecatedClass>
1797-
<code><![CDATA[\OC_Util::setupFS($user)]]></code>
1798-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1799-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1800-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1801-
</DeprecatedClass>
1802-
<DeprecatedMethod>
1803-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1804-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1805-
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
1806-
</DeprecatedMethod>
1807-
</file>
18081795
<file src="apps/files_trashbin/lib/Command/CleanUp.php">
18091796
<DeprecatedClass>
18101797
<code><![CDATA[\OC_Util::setupFS($uid)]]></code>

0 commit comments

Comments
 (0)