[Bug 35558] New: Do not fetch local image if none exists
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Bug ID: 35558 Summary: Do not fetch local image if none exists Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Templates Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org Depends on: 35557 On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. Should not we have this info beforehand in order to reduce the number of hits and network traffic? Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35557 [Bug 35557] LoadResultsCovers is not used (staff) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |didier.gautheron@biblibre.c | |om, | |nick@bywatersolutions.com, | |tomascohen@gmail.com Status|ASSIGNED |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 159785 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159785&action=edit Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #2 from Lucas Gass <lucas@bywatersolutions.com> ---
Should not we have this info beforehand in order to reduce the number of hits and network traffic?
Yes. +1 100%. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- (In reply to Lucas Gass from comment #2)
Should not we have this info beforehand in order to reduce the number of hits and network traffic?
Yes. +1 100%.
Ditto! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Barbara Petritsch <barbara.petritsch@wienmuseum.at> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |barbara.petritsch@wienmuseu | |m.at --- Comment #4 from Barbara Petritsch <barbara.petritsch@wienmuseum.at> --- There are no instructions for testing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #1)
However the page load will be slightly slower ofc.
I don't like this. I think the check should be lower if we are going to do it earlier, maybe in searchResults. I don't want to add to that routine, but I also don't think this should be done in the script. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Nick Clemens from comment #5)
(In reply to Jonathan Druart from comment #1)
However the page load will be slightly slower ofc.
I don't like this.
I think the check should be lower if we are going to do it earlier, maybe in searchResults. I don't want to add to that routine, but I also don't think this should be done in the script.
I put it in the controller on purpose. It's not an info we want every time we call searchResults. Why shouldn't it be done in the script? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Barbara Petritsch from comment #4)
There are no instructions for testing
Add local cover image to some of your bibliographic records. Open the browser console, tab "network". Search for records. => Without this patch you will see 1 GET per result => With this patch applied you will see 1 GET per result *with a cover image*. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Nick Clemens from comment #5)
(In reply to Jonathan Druart from comment #1)
However the page load will be slightly slower ofc.
I don't like this.
I think the check should be lower if we are going to do it earlier, maybe in searchResults. I don't want to add to that routine, but I also don't think this should be done in the script.
And it won't be faster to have this code in searchResults ;) The "slowness" (how much?) will be there if we decide to have the info anyway. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #8)
(In reply to Nick Clemens from comment #5)
(In reply to Jonathan Druart from comment #1)
However the page load will be slightly slower ofc.
I don't like this.
I think the check should be lower if we are going to do it earlier, maybe in searchResults. I don't want to add to that routine, but I also don't think this should be done in the script.
And it won't be faster to have this code in searchResults ;)
The "slowness" (how much?) will be there if we decide to have the info anyway.
If we prefetch the count when getting the biblio object we can reduce calls I prefer loading the page and then making the calls because it makes the page load faster - which benefits the user -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> ---
which benefits the user
Unless it overloads the server because it gets too many requests, which is the goal of this approach. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #10)
which benefits the user
Unless it overloads the server because it gets too many requests, which is the goal of this approach.
Is there evidence of this happening in production? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Nick Clemens from comment #11)
(In reply to Jonathan Druart from comment #10)
which benefits the user
Unless it overloads the server because it gets too many requests, which is the goal of this approach.
Is there evidence of this happening in production?
I feel like we should make sure cache expiration timestamps are sent correctly from the svc. So: - Only ask for local covers on things that have them - Only return them from the svc if they have expired on the browser -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi from comment #12)
(In reply to Nick Clemens from comment #11)
(In reply to Jonathan Druart from comment #10)
which benefits the user
Unless it overloads the server because it gets too many requests, which is the goal of this approach.
Is there evidence of this happening in production?
While I haven't had a server overloaded by image requests, it would be nice to reduce the number of unnecessary HTTP requests. It is conceptually painful looking at 20 requests going out when you know 0 are going to return anything on a given page. That CPU time could be put to better use. Prefetching is an interesting idea. I'm not familiar enough with the DBIx::Class internals for prefetching to know how it works exactly, but in theory it might cut down on DB calls.
I feel like we should make sure cache expiration timestamps are sent correctly from the svc. So:
- Only ask for local covers on things that have them - Only return them from the svc if they have expired on the browser
That's a good idea too, although I think it would be a separate ticket. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #14 from Didier Gautheron <didier.gautheron@biblibre.com> --- (In reply to Nick Clemens from comment #11)
(In reply to Jonathan Druart from comment #10)
which benefits the user
Unless it overloads the server because it gets too many requests, which is the goal of this approach.
Is there evidence of this happening in production?
Yes we have. For some reason customers like to set the result display to 100 (it's a syspref) or more and we have 499 errors as a result (with nginx it's a timeout error). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159785|0 |1 is obsolete| | --- Comment #15 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 160059 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160059&action=edit Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #16 from Didier Gautheron <didier.gautheron@biblibre.com> --- About slowdown: I applied it on customers Koha with syspref numSearchResults as high as 500, server nginx HTTP 1.1 Before response time for search.pl (around 1200 results): 16 to 18 seconds. After: 17 to 18 seconds. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #17 from Didier Gautheron <didier.gautheron@biblibre.com> --- Same is need for OPAC -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #18 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 163136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163136&action=edit Bug 35558: Do not retrieve the local image if none exists - OPAC -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #160059|0 |1 is obsolete| | --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 164295 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164295&action=edit Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163136|0 |1 is obsolete| | --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 164296 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164296&action=edit Bug 35558: Do not retrieve the local image if none exists - OPAC Test plan: 1 - Enable system preferences: LocalCoverImages OPACLocalCoverImages 2 - open browser tools network page 3 - Perform an intranet search 4 - Note a request for each bib like: http://localhost:8081/cgi-bin/koha/catalogue/image.pl?thumbnail=1&biblionumber=35 5 - Repeat on opac, same requests 6 - Apply patches 7 - Restart all 8 - Repeat searches, note no fetches 9 - Add local cover images to several biblios in the results 10 - Repeat searches 11 - Note the fetches are only for those records with images 12 - Confirm images still load correctly Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Note: for a library making heavy use of local covers this will have little impact -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable CC| |fridolin.somers@biblibre.co | |m Version(s)|24.05.00 |24.05.00,23.11.06 released in| | --- Comment #23 from Fridolin Somers <fridolin.somers@biblibre.com> --- Nice small perf patch, I backport Pushed to 23.11.x for 23.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to stable |RESOLVED --- Comment #24 from Lucas Gass <lucas@bywatersolutions.com> --- Enhancement will not be backported to 23.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35558 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37425 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37425 [Bug 37425] Deletion of biblio can cause search errors -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org