Bug 7963 - Parallel HTTP requests when checking URLs
Summary: Parallel HTTP requests when checking URLs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Frédéric Demians
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 2959 6267
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-14 13:01 UTC by Frédéric Demians
Modified: 2016-01-05 05:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (6.04 KB, patch)
2012-04-14 13:07 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (5.94 KB, patch)
2012-04-14 16:13 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (5.95 KB, patch)
2012-04-15 09:10 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (6.25 KB, patch)
2012-06-11 12:40 UTC, Frédéric Demians
Details | Diff | Splinter Review
Dependencies (998 bytes, patch)
2012-06-11 12:40 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (6.32 KB, patch)
2012-06-11 16:51 UTC, Frédéric Demians
Details | Diff | Splinter Review
Dependencies (998 bytes, patch)
2012-06-11 16:51 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (7.60 KB, patch)
2012-08-07 07:19 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 7963 Parallel HTTP requests when checking URLs (7.67 KB, patch)
2012-08-07 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (8.07 KB, patch)
2012-08-27 07:41 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 7963 Parallel HTTP requests when checking URLs (8.13 KB, patch)
2012-08-27 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7963 follow-up: die nicely if AnyEvent libraries not installed (1.02 KB, patch)
2012-10-02 14:57 UTC, Paul Poulain
Details | Diff | Splinter Review
[SIGNED-OFF] follow-up: die nicely if AnyEvent libraries not installed (1.16 KB, patch)
2012-10-02 16:29 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Demians 2012-04-14 13:01:57 UTC
Current script check-url.pl checks URL found in 856$u by sending HTTP
requests, one by one. I propose a new script which send multiple
requests simultaneously.

This script is based on AnyEvent and AnyEvent::HTTP CPAN modules.
Comment 1 Frédéric Demians 2012-04-14 13:07:01 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2012-04-14 16:13:59 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2012-04-15 09:10:33 UTC Comment hidden (obsolete)
Comment 4 Dobrica Pavlinusic 2012-06-11 11:02:02 UTC
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
Comment 5 Frédéric Demians 2012-06-11 12:40:17 UTC Comment hidden (obsolete)
Comment 6 Frédéric Demians 2012-06-11 12:40:53 UTC Comment hidden (obsolete)
Comment 7 Dobrica Pavlinusic 2012-06-11 13:32:16 UTC
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.
Comment 8 Frédéric Demians 2012-06-11 13:46:56 UTC
> 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.
Comment 9 Frédéric Demians 2012-06-11 16:51:08 UTC Comment hidden (obsolete)
Comment 10 Frédéric Demians 2012-06-11 16:51:35 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-08-06 14:50:13 UTC
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
Comment 12 Frédéric Demians 2012-08-06 19:20:21 UTC
> 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.
Comment 13 Frédéric Demians 2012-08-07 07:19:41 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2012-08-07 08:21:42 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2012-08-07 08:37:37 UTC
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;
Comment 16 Mason James 2012-08-10 08:18:32 UTC
(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'
Comment 17 Frédéric Demians 2012-08-27 07:39:44 UTC
> - 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...
Comment 18 Frédéric Demians 2012-08-27 07:41:58 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2012-08-27 08:59:07 UTC
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>
Comment 20 Paul Poulain 2012-10-02 14:57:35 UTC
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
Comment 21 Paul Poulain 2012-10-02 14:57:39 UTC Comment hidden (obsolete)
Comment 22 Frédéric Demians 2012-10-02 16:29:11 UTC
Created attachment 12648 [details] [review]
[SIGNED-OFF] follow-up: die nicely if AnyEvent libraries not installed

Good point!
Comment 23 Paul Poulain 2012-10-05 09:45:25 UTC
Patch pushed to master. Could usefully be ported to 3.8
Comment 24 David Cook 2016-01-05 04:34:50 UTC
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.
Comment 25 David Cook 2016-01-05 05:08:25 UTC
In hindsight, it looks like libanyevent-perl was available in Squeeze, so they were halfway there already.

https://packages.debian.org/squeeze/libanyevent-perl