-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move files/ajax/scan.php to background job #20545
Conversation
b422cda
to
d8a443b
Compare
*/ | ||
protected function runScanner(IUser $user) { | ||
$scanner = new Scanner($user->getUID(), $this->dbConnection); | ||
$scanner->scan(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@icewind1991 @PVince81 Please advise whether we should use \OC\Files\Utils\Scanner::backgroundScan
or \OC\Files\Utils\Scanner::scan
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially as this will be run all 10 minutes for a chunk of 500 users. 😉
looks good. 500 users every 10min sounds reasonable |
// Add cron job for scanning user storages | ||
$jobList = \OC::$server->getJobList(); | ||
$job = 'OCA\Files\BackgroundJob\ScanFiles'; | ||
if(!$jobList->has($job, '')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add it, the JobList takes care of deduplication:
https://github.com/owncloud/core/blob/master/lib/private/backgroundjob/joblist.php#L54-L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. THX.
d8a443b
to
5ba2151
Compare
#20545 (comment) is still open |
Add some tests for the iterating over users cases?
|
protected function runScanner(IUser $user) { | ||
try { | ||
$scanner = new Scanner($user->getUID(), $this->dbConnection); | ||
$scanner->scan(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scan.php uses $scanner->backgroundScan();
which is also what should be used here.
There different is that backgroundScan
only checks for folders that are not fully scanned yet (marked with size -1) while scan
goes trough the entire filesystem of the user.
Using scan
in a backgroundjob in such a naive way will not work when you scale up, when external storages are used scan
can easily take many minutes every time you call it, backgroundScan
will only take time if there's work to be done
Rebase needed |
The background job will now be executed in chunks of 500 users all 10 minutes.
5ba2151
to
391bc49
Compare
Rebased also added a new unit test.
@icewind1991 I'm not entirely sure how to do that here considering the mocking and DIs. So what I just added now is a test that it goes back to the first user once it has reached all. If you have an idea on how to test this any help is utmost appreciated. |
Tested and works 👍 |
Move files/ajax/scan.php to background job
Remove unneeded markup for scan process - ref #20545
The background job will now be executed in chunks of 500 users all 10 minutes.
@DeepDiver1975 @icewind1991 @PVince81 Please review - THX.
Fixes #17906