Bug 8233 - New search engine layer - introduce solr without breaking anything else
Summary: New search engine layer - introduce solr without breaking anything else
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.10
Hardware: All All
: P2 major (vote)
Assignee: Jonathan Druart
QA Contact: Mason James
URL:
Keywords:
: 5360 (view as bug list)
Depends on:
Blocks: 11196 12538
  Show dependency treegraph
 
Reported: 2012-06-11 14:28 UTC by claire.hernandez@biblibre.com
Modified: 2015-06-04 23:23 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
SearchEngine: Add a Koha::SearchEngine module (95.66 KB, patch)
2012-06-13 11:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8233 : SearchEngine: Add a Koha::SearchEngine module (95.77 KB, patch)
2012-06-17 17:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233 fixing the number in updatedatabase.pl (1.03 KB, patch)
2012-06-17 17:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233: Replace the given statement with for (1.35 KB, patch)
2012-06-22 14:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8233: Hide the search engine configuration link for Zebra (1.78 KB, patch)
2012-06-25 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8233 : SearchEngine: Add a Koha::SearchEngine module (95.80 KB, patch)
2012-06-25 09:09 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233 fixing the number in updatedatabase.pl (1.03 KB, patch)
2012-06-25 09:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233: Replace the given statement with for (1.40 KB, patch)
2012-06-25 09:12 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233: Hide the search engine configuration link for Zebra (1.85 KB, patch)
2012-06-25 09:14 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8233: Followup: Search engine layer: Add dependencies. (1.67 KB, patch)
2012-06-25 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
MT8233: We can't load test on Koha::SearchEngine::* (751 bytes, patch)
2012-07-25 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8233: We can't load test on Koha::SearchEngine::* (944 bytes, patch)
2012-07-25 12:39 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8233 - SolR-related /etc/ files break deb building (1.30 KB, patch)
2012-09-17 19:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8233 - SolR-related /etc/ files break deb building (1.36 KB, patch)
2012-09-21 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2012-06-11 14:28:10 UTC
Introduce the Koha::SearchEngine work (hackfest12 kohacon12) and a first poc with Solr. The goal is to break nothing and provide a first work wich will be enhance.

Please use the wiki or koha-devel for discussion:
http://wiki.koha-community.org/wiki/SearchEngine_Layer_RFC
Comment 1 Jonathan Druart 2012-06-13 11:54:24 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-06-17 17:42:37 UTC
I tested the solr work at kohacon12 and it worked very well.

I note that this patch allows us to toggle the syspref and use solr, and toggle it back and be back using zebra.

Currently it only effects the opac search, but that is a great place to start. I see no harm in getting this into master as soon as possible so others can work on the Koha::SearchEngine::Zebra more easily. Especially since if you do not choose solr this has no affect on your current system. Exactly the way it should be.
Comment 3 Chris Cormack 2012-06-17 17:48:18 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-06-17 17:50:19 UTC
I have signed off, and will now send a couple of follow ups to fix updatedatabase and few unconditional warns.

But I would urge people to not fail QA on those issues, it is more important to get this up so it can be tested by lots of people.. now while we still have 4 months before the 3.10.0 release.
Comment 5 Chris Cormack 2012-06-17 17:56:06 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2012-06-17 17:59:53 UTC
Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which is totally still being developed, so I think leaving them unqualified for now is ok and as that code is fleshed out and developed we can remove them.
Comment 7 Jared Camins-Esakov 2012-06-17 18:04:02 UTC
(In reply to comment #6)
> Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which
> is totally still being developed, so I think leaving them unqualified for
> now is ok and as that code is fleshed out and developed we can remove them.

For what it's worth, I think unconditional warns in Koha::SearchEngine::Zebra::* are a good idea.
Comment 8 Jared Camins-Esakov 2012-06-17 18:04:55 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Actually the warns are only in the Koha::SearchEngine::Zebra::* area, which
> > is totally still being developed, so I think leaving them unqualified for
> > now is ok and as that code is fleshed out and developed we can remove them.
> 
> For what it's worth, I think unconditional warns in
> Koha::SearchEngine::Zebra::* are a good idea.

It *is* a good idea. Sorry, I haven't eaten lunch yet and apparently that's affecting my grammar.
Comment 9 Magnus Enger 2012-06-18 06:40:10 UTC
*** Bug 5360 has been marked as a duplicate of this bug. ***
Comment 10 Ian Walls 2012-06-21 18:57:43 UTC
Display of Solr configuration page in Admin is unconditional; will show if Zebra is the search engine... not sure if that's a bad thing or not.

opac/opac-search.pl makes use of "highly experimental" Perl syntax: given and when.  http://perldoc.perl.org/perlsyn.html#Switch-Statements.  While this should be compatible with Koha's minimum Perl version (5.10), it's still listed as experimental in 5.16, which makes me nervous.  I'm all for switch statements, but they're not as fundamental in Perl as they are in other languages, and for an either/or situation like this, doing it another way would be safer.

Reintroduces Test::MockModule

Given the above, I'm not ready to pass this through QA, but I'm not entirely convinced it should fail, either.  Replacing "given" with "for", and adding a 'default' case should clear up the issue.

Otherwise, all the code is new and in it's own area, making it delightfully low-impact on folks who aren't ready to experiment with Solr support.
Comment 11 Jonathan Druart 2012-06-22 08:58:56 UTC
Hi Ian, 
Thanks for your review.

About Switch vs given statements, Switch is buggy and deprecated : "Switch is buggy and should be avoided. You may find Perl's new given /when feature a suitable replacement."

http://perldoc.perl.org/perl5120delta.html#Deprecations
Comment 12 Ian Walls 2012-06-22 13:35:12 UTC
Right, I wasn't advocating for Switch itself, so much as supporting the syntactic structure of switch that can be found in many programming languages.  Looks like Perl does this most stably (right now) with this structure:

for ($var) {
  when (/^abc/) { $abc = 1 }
  when (/^def/) { $def = 1 }
  when (/^xyz/) { $xyz = 1 }
  default { $nothing = 1 }
}

and, "highly experimentally", with

given ($var) {
  when (/^abc/) { $abc = 1 }
  when (/^def/) { $def = 1 }
  when (/^xyz/) { $xyz = 1 }
  default { $nothing = 1 }
}

While I'm sure using given would in most cases be safe, it could very well introduce a difficult-to-trace problem for a non-typical install (where a different Perl version could have snuck in).  I'm probably being overly paranoid, but that's what a QAM is for!
Comment 13 Jonathan Druart 2012-06-22 14:37:58 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2012-06-25 08:06:48 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2012-06-25 09:09:37 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2012-06-25 09:10:52 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2012-06-25 09:12:03 UTC Comment hidden (obsolete)
Comment 18 Chris Cormack 2012-06-25 09:14:19 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2012-06-25 09:15:06 UTC
For replacing the when, and hiding the configuration both work well. Back to signed off
Comment 20 Ian Walls 2012-06-25 14:16:31 UTC
QA comments have been addressed in followup patches, thank you!

One additional issue not caught by earlier scans:  the new modules introduce dependencies on Moose (and sub-modules), Test::MockModule and Data::Pagination, none of which are found on my default install.  These dependencies will need to be added to koha_perl_deps.pl and the Debian packages lists (assuming they are indeed packaged).

Please provide this additional followup, and then we should safe to push this to master, and debug further from there.
Comment 21 Jonathan Druart 2012-06-25 14:56:21 UTC
Hi Ian, 

I don't know if we want to add all solr's dependencies for koha currently.

If I add Data::Pagination, I would have to add all of dependencies listed on http://wiki.koha-community.org/wiki/SearchEngine_Layer_RFC#Install_perl_libs.

And I think it is not what we want for Zebra's installations, isn't it ?

Maybe we want a test in C4::Installer::PerlDependencies to distinguish the search engine ?
Comment 22 Ian Walls 2012-06-25 15:00:13 UTC
The dependencies can be marked as optional and for what purpose, so if folks don't want to install them, they don't need to.

A more complex set up of looking install options and picking dependencies based on that would be nice, but probably more work than we need right now, with our level of available customization.  But, say when Koha is RDBMS-agnostic, then we may find it handy to have different dependency paths in the scripts.
Comment 23 Jonathan Druart 2012-06-25 15:19:20 UTC Comment hidden (obsolete)
Comment 24 Paul Poulain 2012-07-06 14:59:05 UTC
(In reply to comment #20)
> Please provide this additional followup, and then we should safe to push
> this to master, and debug further from there.

Follow-up provided, I agree it passes QA now, so pushing !
Comment 25 Paul Poulain 2012-07-06 15:00:06 UTC
Checked again that I can't see any difference when we're running zebra
Comment 26 Chris Cormack 2012-07-06 20:18:07 UTC
not ok 125 - use Koha::SearchEngine::Zebra;
#   Failed test 'use Koha::SearchEngine::Zebra;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Zebra'.
#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC (@INC contains: /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /usr/share/perl5/Module/Runtime.pm line 205.
#  at /usr/lib/perl5/Moose.pm line 63
# Compilation failed in require at (eval 1313) line 2.
# BEGIN failed--compilation aborted at (eval 1313) line 2.
Bail out!  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'
# Tests were run but no plan was declared and done_testing() was not seen.

Is what is causing jenkins to complain, I will fix it on jenkins but we probably need a patch to fix it properly pretty soon.
Comment 27 Chris Cormack 2012-07-06 20:47:36 UTC
Ok that's fixed (missing modules, easy fix)

However now I am getting this

not ok 129 - use Koha::SearchEngine::Solr;
#   Failed test 'use Koha::SearchEngine::Solr;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Solr'.
#     Error:  Illegal inherited options => (is) at /usr/lib/perl5/Moose/Meta/Class.pm line 646.
#       Moose::Meta::Class::_process_inherited_attribute('Moose::Meta::Class=HASH(0x5a38b40)', 'url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose/Meta/Class.pm line 628
#       Moose::Meta::Class::_process_attribute('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose/Meta/Class.pm line 300
#       Moose::Meta::Class::add_attribute('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'required', 1, 'builder', '_build_url', 'isa', 'Str', 'definition_context', 'HASH(0x5e9bba8)', 'lazy', 1, 'is', 'ro') called at /usr/lib/perl5/Moose.pm line 70
#       Moose::has('Moose::Meta::Class=HASH(0x5a38b40)', '+url', 'is', 'ro', 'isa', 'Str', 'lazy', 1, 'builder', '_build_url', 'required', 1) called at /usr/lib/perl5/Moose/Exporter.pm line 294
#       Moose::has('+url', 'is', 'ro', 'isa', 'Str', 'lazy', 1, 'builder', '_build_url', 'required', 1) called at Koha/SearchEngine/Solr.pm line 7
#       require Koha/SearchEngine/Solr.pm called at (eval 1428) line 2
#       main::BEGIN() called at Koha/SearchEngine/Solr.pm line 0
#       eval {...} called at Koha/SearchEngine/Solr.pm line 0
#       eval 'package main;
# use Koha::SearchEngine::Solr @{$args[0]};
# 1;
#
# ;' called at /usr/local/share/perl/5.10.1/Test/More.pm line 885
#       Test::More::_eval('package main;\x{a}use Koha::SearchEngine::Solr @{$args[0]};\x{a}1;\x{a}', 'ARRAY(0x3f55558)') called at /usr/local/share/perl/5.10.1/Test/More.pm line 860
#       Test::More::use_ok('Koha::SearchEngine::Solr') called at t/00-load.t line 46
#       main::__ANON__() called at /usr/share/perl/5.10/File/Find.pm line 958
#       File::Find::_find_dir('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha', 7) called at /usr/share/perl/5.10/File/Find.pm line 722
#       File::Find::_find_opt('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha') called at /usr/share/perl/5.10/File/Find.pm line 1297
#       File::Find::find('HASH(0x8c2f60)', '/home/jenkins/jobs/Koha_master/workspace/Koha') called at t/00-load.t line 49
# Compilation failed in require at (eval 1428) line 2.
# BEGIN failed--compilation aborted at (eval 1428) line 2.
Bail out!  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Solr'
Comment 28 Jonathan Druart 2012-07-09 09:10:05 UTC
>#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC
> Ok that's fixed (missing modules, easy fix)

Hi Chris!
How have you fixed that ?
I can't reproduce your second error:

% export PERL5LIB=/home/koha/src

% prove t/00-load.t
t/00-load.t .. 15/? Too late to run INIT block at /home/koha/src/C4/Barcodes/hbyymmincr.pm line 41.
Too late to run INIT block at /home/koha/src/C4/External/BakerTaylor.pm line 42.

#   Failed test 'use Koha::SearchEngine::Zebra;'
#   at t/00-load.t line 46.
#     Tried to use 'Koha::SearchEngine::Zebra'.
#     Error:  Can't locate Data/SearchEngine/Zebra.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib
[...]
/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .). at /usr/lib/perl5/Class/MOP.pm line 135
Bailout called.  Further testing stopped:  ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'
# Tests were run but no plan was declared and done_testing() was not seen.
FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'Koha::SearchEngine::Zebra'

I get the same error. But if I export the good path:

% git clone git://github.com/xercode/Data-SearchEngine-Zebra.git /home/koha/Data-SearchEngine-Zebra
% export PERL5LIB="/home/koha/src:/home/koha/Data-SearchEngine-Zebra"
% prove t/00-load.t
t/00-load.t .. 1/? Too late to run INIT block at /home/koha/src/C4/Barcodes/hbyymmincr.pm line 41.
Too late to run INIT block at /home/koha/src/C4/External/BakerTaylor.pm line 42.
t/00-load.t .. ok     
All tests successful.
Files=1, Tests=148,  1 wallclock secs ( 0.04 usr  0.00 sys +  1.00 cusr  0.06 csys =  1.10 CPU)
Result: PASS
Comment 29 Chris Cormack 2012-07-09 09:26:49 UTC
Yes, I fixed the first error by adding the missing module to the server that jenkins is running on.

It still is totally busted on Jenkins, with that second error, and builds won't continue until it is fixed.

I suspect the version of Moose, or MooseX::Something is too low on debian squeeze.
Will investigate more
Comment 30 Chris Cormack 2012-07-09 09:55:18 UTC
Yes, it's moose is too old in debian squeeze. We should test this in wheezy, to make sure it will work with the next release.

But jenkins can run the tests again now.
Comment 31 Jared Camins-Esakov 2012-07-17 19:54:40 UTC
This new code introduces some rather serious problems. The packages required for Solr do not work on Squeeze, which means that t/00-load.t bails out without some serious black magic. Since "all tests should pass" is supposed to be one of the things that people who are signing off and doing QA are checking, this is a huge problem. In order to work around it when signing off, I ran relevant tests individually, but that is not a long-term solution, and certainly will not be a viable option when it comes time to actually release 3.10. Perhaps the packaging manager could be told which modules need to be packaged for Squeeze, so that we could get Koha working again?
Comment 32 claire.hernandez@biblibre.com 2012-07-18 06:55:20 UTC
Jared, you're right. With Robin at Kohacon12, we did some packages I put here for the moment (except xml::easy wich is a core lib) https://depot.biblibre.com/chernandez/lib/all/ you can give it a try. To be really sure, I would need to test on a fresh install. I'll have a look.
Comment 33 Paul Poulain 2012-07-18 08:18:26 UTC
(In reply to comment #31)
> This new code introduces some rather serious problems. The packages required
> for Solr do not work on Squeeze, which means that t/00-load.t bails out
> without some serious black magic. Since "all tests should pass" is supposed
> to be one of the things that people who are signing off and doing QA are
> checking, this is a huge problem. In order to work around it when signing
> off, I ran relevant tests individually, but that is not a long-term
> solution, and certainly will not be a viable option when it comes time to
> actually release 3.10. Perhaps the packaging manager could be told which
> modules need to be packaged for Squeeze, so that we could get Koha working
> again?

As solr stuff is experimental, couldn't we just skip Koha::SearchEngine::Solr for now, like what is done for cache:
return if $m =~ /Cache/; # Cache modules are a WIP, add the tests back when we are using them more

?
Comment 34 Jonathan Druart 2012-07-25 12:22:45 UTC Comment hidden (obsolete)
Comment 35 Jared Camins-Esakov 2012-07-25 12:39:12 UTC Comment hidden (obsolete)
Comment 36 Paul Poulain 2012-08-01 11:54:15 UTC
QA comment: tiny follow-up to please jenkins, passed QA

(but I agree it's a workaround, valid because solr is experimental)
Comment 37 Tomás Cohen Arazi 2012-09-17 19:58:35 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2012-09-20 17:20:29 UTC
Please sign so we can have .deb packages.
Comment 39 Jonathan Druart 2012-09-21 08:32:18 UTC
Created attachment 12408 [details] [review]
Bug 8233 - SolR-related /etc/ files break deb building

Remove them using debian/rules when building .deb packages until
we figure out what to do with them in a .deb install.

To do this I ran the build script like this:

sudo DEB_BUILD_OPTIONS=nocheck ./debian/build-git-snapshot -distribution precise -D precise -r ~/ubuntu -v 3.9.0-046~git -d

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 40 Jonathan Druart 2012-09-21 08:33:24 UTC
I didn't test but the code seems safe
Comment 41 Mason James 2012-10-03 00:33:11 UTC
(In reply to comment #39)
> Created attachment 12408 [details] [review]
> Bug 8233 - SolR-related /etc/ files break deb building
> 
> Remove them using debian/rules when building .deb packages until
> we figure out what to do with them in a .deb install.
> 
> To do this I ran the build script like this:
> 
> sudo DEB_BUILD_OPTIONS=nocheck ./debian/build-git-snapshot -distribution
> precise -D precise -r ~/ubuntu -v 3.9.0-046~git -d
> 
> Sponsored-by: Universidad Nacional de Córdoba
> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>


i'm happy to pass QA on this patch


$ koha-qa.pl -c 1

testing 1 commit(s) (applied to commit 0a35b26)
 * 626c660 Bug 8233 - SolR-related /etc/ files break deb building
      debian/rules
Comment 42 Paul Poulain 2012-10-05 10:00:18 UTC
follow-up Bug 8233 - SolR-related /etc/ files break deb building pushed to master
Comment 43 Marc Véron 2012-10-13 07:11:54 UTC
Question:

Attachment #10480 [details] brings in the follwoing dependency: 
+    'JSON::Any' => {
+        'usage'    => 'Core',
+        'required' => '0',
+        'min_ver'  => '1.28',
+    },

JSON::Any seems to be deprecated, see:
http://search.cpan.org/~perigrin/JSON-Any-1.29/lib/JSON/Any.pm#DEPRECATION

Where is it used?

Marc
Comment 44 Jonathan Druart 2012-10-18 14:02:25 UTC
(In reply to comment #43)
> Question:
> 
> Attachment #10480 [details] brings in the follwoing dependency: 
> +    'JSON::Any' => {
> +        'usage'    => 'Core',
> +        'required' => '0',
> +        'min_ver'  => '1.28',
> +    },
> 
> JSON::Any seems to be deprecated, see:
> http://search.cpan.org/~perigrin/JSON-Any-1.29/lib/JSON/Any.pm#DEPRECATION
> 
> Where is it used?
> 
> Marc

Hi Marc,

JSON::Any is recommended by MooseX::Storage

aptitude show libmoosex-storage-perl
Depend: perl, libmoose-perl (>= 0.99), libstring-rewriteprefix-perl
Recommande: libjson-any-perl, libyaml-perl

I think it is why I added it
Comment 45 Chris Cormack 2013-04-21 00:44:48 UTC
Released in 3.10.0