Bug 7963

Summary: Parallel HTTP requests when checking URLs
Product: Koha Reporter: Frédéric Demians <f.demians>
Component: Command-line UtilitiesAssignee: 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   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
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
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