Skip to content
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

Avoid folder processing on pre-generate #73

Merged
merged 1 commit into from
Sep 26, 2017
Merged

Conversation

seik
Copy link
Contributor

@seik seik commented Aug 27, 2017

This in theory fixes #62.

When you upload a file (or a folder) the folder/s and the files are added to the db, so if I'm not missing something there is no need to process folders.

Furthermore related with #62, on my end I could find that the duplicate folder was the root user folder ex: /data/exampleuser/ (I can't really say why the root folder was passed to the postWrite() method on Watcher.php) so as you can imagine the processFolder() method tried to scan the entire user folder multiple times.

@codecov-io
Copy link

Codecov Report

Merging #73 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #73     +/-   ##
=========================================
+ Coverage         5%   5.2%   +0.2%     
- Complexity       62     63      +1     
=========================================
  Files             6      6             
  Lines           260    250     -10     
=========================================
  Hits             13     13             
+ Misses          247    237     -10
Impacted Files Coverage Δ Complexity Δ
lib/Command/PreGenerate.php 0% <ø> (ø) 23 <0> (ø) ⬇️
lib/Watcher.php 100% <100%> (ø) 4 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3faf9a2...58aa621. Read the comment docs.

@MichaIng
Copy link
Member

I am not too good in PHP coding to understand everything, but first I though processFolder() is about creating the previews of the folder itself (which consists of some previews of pictures inside patched together), e.g. seen on gallery app.

But as I see now, it really just seems to process all files inside the folder. So if your assumption is write, that by adding a folder with files inside, the watcher also reacts on each included file, this PR makes totally sense. Should be easy to find out by adding some test folder+pictures inside and run occ preview:pre-generate in verbose mode 🙂 . I hope to find some time for this later.

@seik
Copy link
Contributor Author

seik commented Sep 23, 2017

Hi @MichaIng! Is there any chance of reviewing this PR? A lot of people will benefit from this. A the moment pre-generate scans all the folders and people with small instances (like Raspberry PI's).

@MichaIng
Copy link
Member

I just tested to add a folder with two images inside and ran occ preview:pre-generate with the result that indeed the files were scanned double. Also by seeing the timestamps it is clear that there were no actual previews generated for the folder (e.g. the folders view in gallery app), just the previews for the images inside were generated. Afterwards the two images were scanned again but no previews were generated anymore, reasonable because they were made already before:

2017-09-24 15:00:03 Generating previews for folder /Micha/files/Verkäufe
2017-09-24 15:00:03 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175139.jpg
2017-09-24 15:00:37 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175206.jpg
2017-09-24 15:01:10 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175139.jpg
2017-09-24 15:01:10 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175206.jpg

So I can confirm, that your assumptions for the PR are right and will add and test the PR tomorrow.

@MichaIng
Copy link
Member

MichaIng commented Sep 25, 2017

I just applied the PR to my app, removed the test folder, recreated it again and ran pre-generate:

2017-09-25 13:00:03 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175139.jpg
2017-09-25 13:00:36 Generating previews for /Micha/files/Verkäufe/IMG_20170815_175206.jpg

Previews for both files were generated and they were just touched once for this 👍 ! Vote 4 merging this PR!

This shows by the way that previews were removed together with the files, as expected. This was also doubted here and there on help.nextcloud.com 😉.

€: I just published the PR on the forum and hope for some more testers and attention from the devs by this: @https://help.nextcloud.com/t/more-efficient-pre-generation-github-pull-request/21475

@seik
Copy link
Contributor Author

seik commented Sep 26, 2017

Thank you @MichaIng for the testing 😁. Looking forward to a PR review of @rullzer when he has the time.

@rullzer
Copy link
Member

rullzer commented Sep 26, 2017

@seik nice stuff. I was thinking about something similar this week as I saw the table getting pretty full at my own server. Lets get this in!

@rullzer rullzer merged commit 394b7e8 into nextcloud:master Sep 26, 2017
@rullzer rullzer added this to the 1.0.7 milestone Sep 26, 2017
@rullzer
Copy link
Member

rullzer commented Sep 26, 2017

Released in 1.0.7! Thnx again :D

@seik seik mentioned this pull request Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

occ preview:pre-generate don't stop after hours of execution
4 participants