Summary: | Parallel HTTP requests when checking URLs | ||
---|---|---|---|
Product: | Koha | Reporter: | Frédéric Demians <f.demians> |
Component: | Command-line Utilities | Assignee: | Frédéric Demians <f.demians> |
Status: | CLOSED FIXED | QA Contact: | Ian Walls <koha.sekjal> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | chris, dcook, dpavlin, jonathan.druart, mtj, paul.poulain |
Version: | 3.10 | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 2959, 6267 | ||
Bug Blocks: | |||
Attachments: |
Proposed patch
Proposed patch Proposed patch Proposed patch Dependencies Proposed patch Dependencies Proposed patch Bug 7963 Parallel HTTP requests when checking URLs Proposed patch Bug 7963 Parallel HTTP requests when checking URLs Bug 7963 follow-up: die nicely if AnyEvent libraries not installed [SIGNED-OFF] follow-up: die nicely if AnyEvent libraries not installed |
Description
Frédéric Demians
2012-04-14 13:01:57 UTC
Created attachment 9195 [details] [review] Proposed patch Created attachment 9197 [details] [review] Proposed patch Created attachment 9206 [details] [review] Proposed patch Works nicely, but here are two nit-picks: * script should be named check-url-parallel or something similar so that users know why we have two scripts with almost same name but not quite * --host-pro should probably be --host-intranet to better convey meaning of flag Created attachment 10194 [details] [review] Proposed patch Thanks Dobrica for your comments that make sense. Created attachment 10195 [details] [review] Dependencies You still love --host-pro flag I guess :-) I needed to read source code twice to figure out which URL this is so I really think that flag name needs change. I had a quick chat with Chris about dependencies on AnyEvent::HTTP which isn't part of Debian stable and came out with solution to mark both AnyEvent and AnyEvent::HTTP as optional dependency (using required => 0) so that we don't bring in additional modules but still offer then as suggested in Debian package. > You still love --host-pro flag I guess :-) Too bad! I've changed the help text, and forgot to change the code. I will do that this evening, quietly. > I had a quick chat with Chris about dependencies on AnyEvent::HTTP which > isn't part of Debian stable and came out with solution to mark both AnyEvent > and AnyEvent::HTTP as optional dependency (using required => 0) so that we > don't bring in additional modules but still offer then as suggested in > Debian package. Will change dependency patch accordingly. Thanks. Created attachment 10238 [details] [review] Proposed patch Created attachment 10239 [details] [review] Dependencies QA Comments (before Sign-off): 1/ Why don't you remove the old script? (check-url.pl) 2/ Please replace +use 5.010; +use strict; +use warnings; with use Modern::Perl; 3/ It seems in Koha, the scripts have an extension. Please add the .pl 4/ It would be great to pass an argument (or syspref :-/) to the script containing a list of field to check ? In UNIMARC, we can have 316$u, 317$u, 464$u, etc. with urls. 5/ For a new script it is very nice to submit the patch after a perltidy ;) 1, 3 and 5 are mandatory to pass QA > 1/ Why don't you remove the old script? (check-url.pl) To avoid any regression to any library using the old script in a way I can't figure out. Maybe later? > 3/ It seems in Koha, the scripts have an extension. Please add the .pl I don't like this rule but I can follow it... > 4/ It would be great to pass an argument (or syspref :-/) to the script > containing a list of field to check ? In UNIMARC, we can have 316$u, 317$u, > 464$u, etc. with urls. Good suggestion. > 5/ For a new script it is very nice to submit the patch after a perltidy ;) Will send a new version in few weeks. Created attachment 11403 [details] [review] Proposed patch Created attachment 11405 [details] [review] Bug 7963 Parallel HTTP requests when checking URLs Current script check-url.pl checks URL found in 856$u by sending HTTP requests, one by one. The next request can't be sent before the previous one get a result, which can be very slow for dead URL. I propose a new script which send multiple requests simultaneously which improve drastically URL checking execution time. This script is based on AnyEvent and AnyEvent::HTTP CPAN modules. Add new dependencies AnyEvent & AnyEvent::HTTP. See doc: perldoc check-url-quick.pl Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Hi Frédéric, Thank you for submitting this patch such fast :) It works very well in all my tests. Just somes little remarks: - POD is not up to date (--tags in summary, -h is not supported, "Scan all URLs found in 856$u" or in others tags :)) - Perhaps a modification in the other script is necessary (to indicate that an other one is available). - I think the following lines my %h = map { $_ => undef } @tags; @tags = sort keys %h; could be replaced with: @tags = sort @tags; (In reply to comment #15) > Hi Frédéric, > > Thank you for submitting this patch such fast :) > It works very well in all my tests. > > Just somes little remarks: status modded to 'in discussion' > - POD is not up to date (--tags in summary, -h is not supported, "Scan all > URLs found in 856$u" or in others tags :)) > - Perhaps a modification in the other script is necessary (to indicate that > an other one is available). Thanks. I will submit an improved version of this patch. > - I think the following lines > my %h = map { $_ => undef } @tags; > @tags = sort keys %h; > could be replaced with: > @tags = sort @tags; No. This 2 lines are there to deduplicate tags, in any case... Created attachment 11837 [details] [review] Proposed patch Created attachment 11840 [details] [review] Bug 7963 Parallel HTTP requests when checking URLs Current script check-url.pl checks URL found in 856$u by sending HTTP requests, one by one. The next request can't be sent before the previous one get a result, which can be very slow for dead URL. I propose a new script which send multiple requests simultaneously which improve drastically URL checking execution time. This script is based on AnyEvent and AnyEvent::HTTP CPAN modules. Add new dependencies AnyEvent & AnyEvent::HTTP. See doc: perldoc check-url-quick.pl Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> QA comment: * passes koha-qa.pl * badly fail if you don't have AnyEvent packages installed. I wrote a small follow-up for a better message in case of problem. Please sign it, and mark passed QA, everything is OK Created attachment 12644 [details] [review] Bug 7963 follow-up: die nicely if AnyEvent libraries not installed Created attachment 12648 [details] [review] [SIGNED-OFF] follow-up: die nicely if AnyEvent libraries not installed Good point! Patch pushed to master. Could usefully be ported to 3.8 I'm thinking about using AnyEvent::HTTP for bug 10662, and I see that libanyevent-http-perl is now a part of Debian Stable (Jessie) and Debian Old Stable (Wheezy). https://packages.debian.org/wheezy/libanyevent-http-perl I wonder if we should now include it as a required rather than optional dependency? For my purposes, I could probably just display a message telling users that they need to install the library before they can use the feature, but it'd probably be nice to have it already available. That said, I haven't decided whether or not to use AnyEvent::HTTP quite yet. Just one of a few options I'm pondering... but thought I'd mention how it's now in Debian. In hindsight, it looks like libanyevent-perl was available in Squeeze, so they were halfway there already. https://packages.debian.org/squeeze/libanyevent-perl |