Bug 11081 - Port Koha::Contrib::Tamil indexer into Koha code base
Summary: Port Koha::Contrib::Tamil indexer into Koha code base
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Keywords: dependency
Depends on: 8773
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-20 07:59 UTC by Frédéric Demians
Modified: 2019-10-14 19:56 UTC (History)
13 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 (22.65 KB, patch)
2013-10-20 08:01 UTC, Frédéric Demians
Details | Diff | Splinter Review
Port Koha::Contrib::Tamil indexer into Koha code base (23.02 KB, patch)
2015-05-18 06:37 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11081: Port Koha::Contrib::Tamil indexer into Koha code base (23.27 KB, patch)
2015-05-18 11:02 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 11081: (followup) Add license information (5.98 KB, patch)
2015-05-18 11:02 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11081: (followup) Add license information (6.03 KB, patch)
2015-05-20 07:28 UTC, Frédéric Demians
Details | Diff | Splinter Review
[PASSED QA] Bug 11081: Port Koha::Contrib::Tamil indexer into Koha code base (23.34 KB, patch)
2015-05-22 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11081: (followup) Add license information (6.12 KB, patch)
2015-05-22 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11081: Port Koha::Contrib::Tamil indexer into Koha code base (23.30 KB, patch)
2016-01-02 17:23 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 11081: (followup) Add license information (6.19 KB, patch)
2016-01-02 17:23 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 11081: (followup) Rebuild debian/control (2.80 KB, patch)
2016-02-16 14:10 UTC, Tomás Cohen Arazi
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 2013-10-20 07:59:13 UTC

    
Comment 1 Frédéric Demians 2013-10-20 08:01:07 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2013-10-20 17:09:53 UTC
How does this compare to bug 6435?
Comment 3 Frédéric Demians 2013-10-20 19:23:42 UTC
> How does this compare to bug 6435?

It's a new implementation of indexing: lightweight, surclassable in
order to accommodate SolR, not mixing indexing and data sanitizing,
quicker (equivalent to -xml option). It's a true event driven script,
not sleep(ed) one. It may also have to do with Koha code base
refactoring.

From my perspective, rebuild_zebra.pl has grown too far, doing to much
things.

Take a look at:

  http://wiki.koha-community.org/wiki/Background_indexing_with_Zebra

See also bug 8773
Comment 4 I'm just a bot 2013-10-30 07:39:10 UTC
Patch applied cleanly, go forth and signoff
Comment 5 wajasu 2014-02-08 04:19:01 UTC
I tested your indexer a couple of years back.  I guess if Moose related deps are around now, this can get in.  I liked running what I had back then when testing.  It was nice to catalogue and have things show up quickly.

I wonder if any of the issues relating to Bug 11078 would be handled by this implementation.

Also, if your implementaion is a long running daemon, do you have any stats on the longest uptime of your production site.

Maybe Robin can comment about if this implementation is package friendly. Multiple database instances handled, etc.
Comment 6 Frédéric Demians 2014-02-08 10:21:24 UTC
> I tested your indexer a couple of years back.  I guess if Moose related deps
> are around now, this can get in.  I liked running what I had back then when
> testing.  It was nice to catalogue and have things show up quickly.

rebuild_zebra.pl daemon mode provide that today. See my response to M. Tompsett
to find reasons why this enhancement may be of interest in the medium term.

> I wonder if any of the issues relating to Bug 11078 would be handled by this
> implementation.

For me bug 11078 is a non-issue. If you do a full re-indexing, you HAVE to stop
the indexing daemon, or you have to do it when nobody will update
biblio/authortiy records.

> Also, if your implementaion is a long running daemon, do you have any stats
> on the longest uptime of your production site.

~1 year. It is stopped when Koha instances are upgraded.

> Maybe Robin can comment about if this implementation is package friendly.
> Multiple database instances handled, etc.

See bug 8773.
Comment 7 Bernardo Gonzalez Kriegel 2015-05-17 23:40:53 UTC
Trying to test

Some conflicts, dependencies installed.

When runnin I found:
misc/bin/koha-index --verbose
Exporting records to update
Can't call method "config" on an undefined value at /home/bgkriegel/kohaclone/C4/Context.pm line 640.

Am I doing something wrong?

Some strange POD errors

 FAIL   Koha/Indexer/Daemon.pm
   OK     critic
   OK     forbidden patterns
   FAIL   pod
                *** ERROR: Unknown command 'attr'  in file Koha/Indexer/Daemon.pm
   OK     valid

 FAIL   Koha/Indexer/Indexing.pm
   OK     critic
   OK     forbidden patterns
   FAIL   pod
                *** ERROR: Unknown command 'method'  in file Koha/Indexer/Indexing.pm
                *** ERROR: Unknown command 'HEAD1'  in file Koha/Indexer/Indexing.pm
   OK     valid
Comment 8 Frédéric Demians 2015-05-18 06:37:34 UTC Comment hidden (obsolete)
Comment 9 Frédéric Demians 2015-05-18 06:39:11 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #7)
> Trying to test

Thanks!

> Some conflicts, dependencies installed.

Fixed on the new attached patch.

> When runnin I found:
> misc/bin/koha-index --verbose
> Exporting records to update
> Can't call method "config" on an undefined value at
> /home/bgkriegel/kohaclone/C4/Context.pm line 640.

Fixed.

> Am I doing something wrong?
> 
> Some strange POD errors

Fixed.

I stay available for fixing any issue you may encounter.
Comment 10 Bernardo Gonzalez Kriegel 2015-05-18 11:02:24 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2015-05-18 11:02:48 UTC Comment hidden (obsolete)
Comment 12 Frédéric Demians 2015-05-20 07:28:24 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-05-22 14:44:31 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-05-22 14:44:40 UTC Comment hidden (obsolete)
Comment 15 Brendan Gallagher 2015-12-31 20:27:37 UTC
Didn't apply cleanly for me.  Can you please rebase?
Comment 16 Frédéric Demians 2016-01-02 17:23:03 UTC
Created attachment 46176 [details] [review]
Bug 11081: Port Koha::Contrib::Tamil indexer into Koha code base

Add two scripts for indexing: koha-index and koha-index-daemon.

Documentation:

  perldoc koha-index
  perldoc koha-index-daemon

New dependencies:

  MooseX::Getopt
  MooseX::RW
  AnyEvent::Processor

Available as a Debian package: libmoosex-getopt-perl

Test plan:

- Apply the patch
- Install dependencies, if necessary
- Reindex a whole biblio catalog:
  koha-index --verbose
- Reindex a whole authorities catalog:
  koha-index --source authority --verbose
- Modify 1 biblio record, and index it:
  koha-index --select queue
- Modifiy 1 authority record, and index it:
  koha-index --select queue --source authority
- Run indexing daemon, with a 15s timeout:
  koha-index-daemon --timeout 15
- Modify a biblio record: wait 15s, and see
- Modify an authority record: wait 15s, and see

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Works well, followed test plan without problems

One comment: perhaps an option for koha-index-daemon
to work quietly, without printing information.

No koha-qa errors, but new files don't have license,
fixed in followup

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Frédéric Demians 2016-01-02 17:23:25 UTC
Created attachment 46177 [details] [review]
Bug 11081: (followup) Add license information

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 18 Frédéric Demians 2016-01-02 17:24:14 UTC
Rebased, and revert to Passed QA status.
Comment 19 Brendan Gallagher 2016-01-27 06:32:33 UTC
Pushed to Master - Should be in the May 2016 release. Thanks!
Comment 20 Jonathan Druart 2016-01-29 09:13:37 UTC
After the discussion on bug 11190, I thought we all agreed that Moo must be used, instead of Moose.
Comment 21 Jonathan Druart 2016-01-29 09:16:00 UTC
Time::Progress and String::RewritePrefix are listed in the deps, but don't look to be used from this patch.
Comment 22 Tomás Cohen Arazi 2016-02-16 14:10:39 UTC
Created attachment 48126 [details] [review]
Bug 11081: (followup) Rebuild debian/control

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 23 Tomás Cohen Arazi 2016-02-16 14:12:06 UTC
Brendan and Galen, this patch should update the deps. I put it here just in case you didn't rebuild the control file yet.
Comment 24 Brendan Gallagher 2016-02-16 18:58:37 UTC
Pushed the followup - Thanks!  Should be in the May 2016 release.
Comment 25 Katrin Fischer 2016-02-28 21:37:13 UTC
What's the difference to the existing Indexer and this one?

Also... I am confused about readding Moose too (comment#20)
Comment 26 Chris Cormack 2016-02-28 21:57:21 UTC
(In reply to Jonathan Druart from comment #20)
> After the discussion on bug 11190, I thought we all agreed that Moo must be
> used, instead of Moose.

That was my understanding
Comment 27 Tomás Cohen Arazi 2016-02-29 13:38:13 UTC
(In reply to Chris Cormack from comment #26)
> (In reply to Jonathan Druart from comment #20)
> > After the discussion on bug 11190, I thought we all agreed that Moo must be
> > used, instead of Moose.
> 
> That was my understanding

Same here.
Comment 28 Brendan Gallagher 2016-02-29 15:22:19 UTC
(In reply to Tomás Cohen Arazi from comment #27)
> (In reply to Chris Cormack from comment #26)
> > (In reply to Jonathan Druart from comment #20)
> > > After the discussion on bug 11190, I thought we all agreed that Moo must be
> > > used, instead of Moose.
> > 
> > That was my understanding
> 
> Same here.

hmm...  Should I revert this until it's not using the Moose?  Thanks.
Comment 29 Tomás Cohen Arazi 2016-02-29 15:26:03 UTC
(In reply to Brendan Gallagher from comment #28)
> (In reply to Tomás Cohen Arazi from comment #27)
> > (In reply to Chris Cormack from comment #26)
> > > (In reply to Jonathan Druart from comment #20)
> > > > After the discussion on bug 11190, I thought we all agreed that Moo must be
> > > > used, instead of Moose.
> > > 
> > > That was my understanding
> > 
> > Same here.
> 
> hmm...  Should I revert this until it's not using the Moose?  Thanks.

I think so.
Comment 30 Kyle M Hall 2016-02-29 16:38:56 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> (In reply to Brendan Gallagher from comment #28)
> > (In reply to Tomás Cohen Arazi from comment #27)
> > > (In reply to Chris Cormack from comment #26)
> > > > (In reply to Jonathan Druart from comment #20)
> > > > > After the discussion on bug 11190, I thought we all agreed that Moo must be
> > > > > used, instead of Moose.
> > > > 
> > > > That was my understanding
> > > 
> > > Same here.
> > 
> > hmm...  Should I revert this until it's not using the Moose?  Thanks.
> 
> I think so.

My understanding was that we would not use Moose for CGI scripts as the startup cost is too high for Moose compared to Moo. However, we this is not a CGI script but a daemon, so the startup cost for using Moose is not a serious factor.
Comment 31 Katrin Fischer 2016-02-29 16:44:28 UTC
What are the consequences for the packages? Are the new dependencies available in Debian? My understanding is that although they are marked optional they will have to be included?
Comment 32 Kyle M Hall 2016-02-29 16:55:10 UTC
(In reply to Katrin Fischer from comment #31)
> What are the consequences for the packages? Are the new dependencies
> available in Debian? My understanding is that although they are marked
> optional they will have to be included?

That is correct. I have checked and all the dependancies are already packaged for Debian 8 ( Jessie ).
Comment 33 Kyle M Hall 2016-02-29 17:02:54 UTC
(In reply to Kyle M Hall from comment #32)
> (In reply to Katrin Fischer from comment #31)
> > What are the consequences for the packages? Are the new dependencies
> > available in Debian? My understanding is that although they are marked
> > optional they will have to be included?
> 
> That is correct. I have checked and all the dependancies are already
> packaged for Debian 8 ( Jessie ).

Looks like all the new dependencies for indexer are already provided by Debian for Wheezy and Jessie, or are provided by our own Debian repo.
Comment 34 Katrin Fischer 2016-02-29 21:41:19 UTC
It's still adding yet another dependency ... and quite a heavy one. I haven't looked up the meeting logs yet, so not sure where we left the discussion in the end, but I think this is worth taking a second look.
Comment 35 Kyle M Hall 2016-03-01 17:02:46 UTC
This patch set has been reverted in master. It has been decided that we shall not use Moose in Koha. Please modify this patch set to use Moo instead, and it should make it back in!
Comment 36 Frédéric Demians 2016-03-01 17:58:50 UTC
Too bad. It won't be possible to port this to use Moo in place of Moose. This code rely on Moose functionalities that aren't available with Moo.