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

URL used to retrieve thumbnails can generate "URI too long" error messages [$15 awarded] #27

Closed
5 tasks done
oparoz opened this issue Feb 15, 2015 · 19 comments
Closed
5 tasks done
Assignees
Milestone

Comments

@oparoz
Copy link
Contributor

oparoz commented Feb 15, 2015

Current situation

Initial post: #159
A thumbnail is identified by its path. So if we're 10 levels deep and have lots of sub-folders with long-names, we're going to max out the limit set for GET requests.
Note: We have to use GET requests because we're using Server-sent Events.

Solution

  • Identify thumbnails by their ID
  • Only retrieve what's needed for the current view
  • Use file IDs when requesting thumbnails in batch
  • Use file IDs when requesting large previews and to download files

This requires re-factoring a fair bit of the current code

Next steps

Feasibility study

  • Collect requirements
  • Calculate effort required
  • Build proof of concept

Development

QA

Test will be written after another big upcoming change

The $15 bounty on this issue has been claimed at Bountysource.

@mnlipp
Copy link

mnlipp commented Mar 30, 2015

As you have written yourself in #159 using IDs -- as outlined in the solution -- won't fix the problem for a large number of pictures.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 30, 2015

Actually it will, since when using IDs you will never reach the limit which triggers the error.
Maybe you're referring to a different issue? The current ones related to performance are all listed in #43.

@mnlipp
Copy link

mnlipp commented Mar 30, 2015

So there is a limit regading the maximum number of pictures requested in a single GET? Because else, it is easy to see that approx. (8150 / (ID-length + seperator)) still is an upper bound (with standard Apache settings). I have about 50 GB in a directory stored on my ownCloud server and I never counted, but I guess there are about 5000 pictures in there (icons and thumbs mostly).

@oparoz
Copy link
Contributor Author

oparoz commented Mar 30, 2015

There is a limit to the length of the GET request, so with 10 IDs maximum per request, it's very unlikely that you're going to reach it, whereas currently, using paths, it's not too difficult if you have long folder and pictures names deeply nested.
In Gallery+, you only hit the issue when reaching the deeper folders which doesn't seem to affect that many people based on the number of people participating in this ticket.

@mnlipp
Copy link

mnlipp commented Mar 30, 2015

Well, the request that was logged in my Apache server this morning had definitely more than 10 picture file names in it. There are 80 ';' in the truncated request. So I'm not sure whether you plan to introduce the limit as part of your solution or whether the limit is not working currently. My ownCloud version is 8.0.2.

Regarding participation in this ticket, I think that not all duplicates have been marked as such yet. This applies at least to #159 and #172.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 30, 2015

If you're seeing 80 requests, then you're not using Gallery+ (or there is a serious bug).

I've just revisited those tickets and you're right, I didn't mention in those ones that people needed to switch to this project to get things fixed.

@mnlipp
Copy link

mnlipp commented Mar 30, 2015

Your're right, I didn't switch. Gallery+ doesn't appear in the apps menu (to ease other's life, it can be found here). I've seen that obviously it needs some patching of ownCloud (see here and here). Well, with my installation being updated from the package repository, this is not really an option. File repository and PIM are more important to me than being able to look at the pictures in the browser. So I'll simply wait until Gallery+ becomes part of the main distribution.

@oparoz
Copy link
Contributor Author

oparoz commented Mar 30, 2015

You're highlighting a big problem with the current distribution mechanism. It's more difficult than it should be to install apps. The core team is working on a solution to make it easier to install them, but it may take at least another 6 months before seeing any change.

Regarding patching, it's not required. It just makes the app faster, especially if you have large media files or if you're storing files used in graphics design.

And 3rd party apps are compatible with distribution packages. I don't understand the issues it would create for you. You just download and unpack them in your apps folder and activate them as an admin.

@jospoortvliet
Copy link

@oparoz we'll for sure address much of the app situation with the 8.1 release (2 months from now) but yeah, there's more that will have to wait for 8.2 and so on.

@oparoz
Copy link
Contributor Author

oparoz commented Apr 8, 2015

@jospoortvliet - I can't wait, because it even took me a long time to start installing apps by downloading them as it's a maintenance nightmare to keep track of all of them, so I can't imagine what it's like to regular users, especially the ones who've been using the system for a while since it was possible to install a lot of apps straight from oC in previous incarnations.

@oparoz oparoz changed the title URL used to retrieve thumbnails can generate "URI too long" error messages URL used to retrieve thumbnails can generate "URI too long" error messages [$15] May 24, 2015
@oparoz oparoz added the bounty label May 24, 2015
@oparoz
Copy link
Contributor Author

oparoz commented May 31, 2015

Master is now using IDs.
Please let me know if you run into any issues.
The app should behave as usual for top folders and it should now be possible to reach very deep folders.

@jospoortvliet
Copy link

Well, I'm using master on ownCloud 8.1 and while it's not perfectly fast (way, no) it works. Loading the main folder where all the pics are takes half a minute, and so does starting the app itself at first, but Pictures - well, it starts faster, but never shows any pictures so there isn't much point ;-)

This is really nice work and I look forward to having this app default in ownCloud 8.2!

a big 👍 on this work from me.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 18, 2015

Cool :)

Remember to use the dev branch on 8.1 and when not testing, you need to enable asset pipe-lining in order to reduce the time it takes for the app to load since I've split all the JS classes into individual files. Hopefully, in the future, this won't be needed.

Regarding loading speed, it would also be interesting if you could test this gallery app
https://github.com/libasys/ownCloud-8.1-Gallery-App

He uses a different way to collect the initial data. I believe it will take the same time since in the end we both hit the DB cache, but I'm interested in capturing more feedback.

@jospoortvliet
Copy link

did @libasys just create a 3rd gallery app? aw ;-)

Every gallery app developer should join a nice cage fight at the ownCloud Contributor Conference. The winners' app will be default in ownCloud 9.0 ;-)

@oparoz
Copy link
Contributor Author

oparoz commented Jun 18, 2015

There is no contest, this one wins as it's the only one using the AppFramework ;)

The problem is that he's built a lot of functionalities on the old base, so it's going to get worse and worse, as he adds more, until people won't be able to install it because of core restrictions.
So, hopefully, we'll be able to migrate most of his ideas to this one, but we can't just apply patches.
At some point I might even create my own advanced gallery fork, in order to keep this one simple, but we're not there yet.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 18, 2015

@graywind @mightycoco @abelgomez @Florz
Please test the master branch (grab the zip if you don't want to use git)

You should not encounter the 414 error any more, when going trying to load images from deeply nested folders.

@jospoortvliet
Copy link

Just joking ;-)

Hoping you all will be at the conf, that would be fun.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 19, 2015

I will, but nobody has approved my submission yet 😭

@jospoortvliet
Copy link

@oparoz dude, the CfP ends end of this month, only then will I look at submissions :D

@oparoz oparoz removed the QA-testing label Jun 25, 2015
@oparoz oparoz closed this as completed Jun 25, 2015
@oparoz oparoz changed the title URL used to retrieve thumbnails can generate "URI too long" error messages [$15] URL used to retrieve thumbnails can generate "URI too long" error messages [$15 awarded] Jul 20, 2015
icewind1991 added a commit that referenced this issue Jul 25, 2015
make gallery work after the removal of the shared folder
@oparoz oparoz removed the bounty label Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants