Bug 25292

Created attachment 103795 [details] [review]
Bug 25292: Flushing L1 on every Z3950 server search

This patch flushes the L1 cache on each Z3950 server search.

There is no need to flush it in init_handler (when the connection is
made), the L1 cache is not accessed (this behaviour goes against what I
expected).

This means each search will fetch values from the
L2 cache (ie memcached) and persist the L1 cache (in Perl
memory cache) until the result are returned (or longer?).

Test plan:
% perl misc/z3950_responder.pl --config-dir=/kohadevbox/koha/etc/z3950
% zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search x
ZOOM>search x

Between the 2 searches you can modify the value of SearchEngine, you
will notice that the number of hits is different

I can reproduce the test plan on a dual installation (ES/Zebra), and see the result variation depending on selected search engine. So it works. But won't it impact performance?

Of course there will be a difference, but you will hit L2 (memcached) anyway. It won't be worst than other search (UI).

I am not expecting a huge difference here but I have not compared.

(In reply to Jonathan Druart from comment #1)
> ZOOM>search x
> ZOOM>search x
> 
> Between the 2 searches you can modify the value of SearchEngine, you
> will notice that the number of hits is different

I don't get what should I do to see a difference in the number of hits.

I've searched for few things before the patch.
And after the patch (after restarting the responder). Same number of hits.
Then I did restart_all.

In this session, I switched at some points from ES to Zebra.
I'm confused and can't invent a list of steps that leads to something consistently good (after the patch) or bad (before the patch).


kohadev-koha@6c8a568fb951:/kohadevbox/koha$ zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search shock
localhost:2100: 2 hits
ZOOM>search the
localhost:2100: 192 hits
ZOOM>search the
localhost:2100: 192 hits
ZOOM>search the
localhost:2100: 192 hits
# I think I switched from 
ZOOM>search the
localhost:2100: 0 hits
ZOOM>search the
localhost:2100: 0 hits
ZOOM>search the
localhost:2100: 0 hits
ZOOM>search les
localhost:2100: 0 hits
ZOOM>search yes
localhost:2100: 0 hits
ZOOM>search shoch
localhost:2100: 0 hits
ZOOM>search shodck
localhost:2100: 0 hits
ZOOM>search shock
localhost:2100: 0 hits
ZOOM>search the
localhost:2100: 0 hits


second try with more details.

kohadev-koha@6c8a568fb951:/kohadevbox/koha$ zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search shock
localhost:2100: 2 hits
ZOOM>search the
localhost:2100: 192 hits
ZOOM>
kohadev-koha@6c8a568fb951:/kohadevbox/koha$ # switch toZebra
kohadev-koha@6c8a568fb951:/kohadevbox/koha$ zoomsh                                                                                             
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search shock
localhost:2100 error: Temporary system error (Bib-1:2) Cannot connect to upstream server                                                       
ZOOM>search the
localhost:2100 error: Temporary system error (Bib-1:2) Cannot connect to upstream server                                                       
ZOOM>
kohadev-koha@6c8a568fb951:/kohadevbox/koha$ zoomsh
ZOOM>
kohadev-koha@6c8a568fb951:/kohadevbox/koha$ #restart responder
kohadev-koha@6c8a568fb951:/kohadevbox/koha$ zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search shock
localhost:2100 error: Temporary system error (Bib-1:2) Cannot connect to upstream server
ZOOM>search the
localhost:2100 error: Temporary system error (Bib-1:2) Cannot connect to upstream server
ZOOM>

The last two logs are with the patch applied.

This is all what I do:

ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search x
localhost:2100: 18 hits

# Change the syspref SearchEngine

ZOOM>search x
localhost:2100: 230 hits

This is a ugly way of confirming that the correct value has been retrieve, but you could add a warn statement in the code, in search_handler, to check which value from cache (so easy to test is a syspref) is retrieved.

After changing the syspref, I must restart ZOOM to have a different number of hits.

Is that expected?

(In reply to Victor Grousset/tuxayo from comment #7)
> After changing the syspref, I must restart ZOOM to have a different number
> of hits.
> 
> Is that expected?

No, please detail what you tried and the result.

(In reply to Jonathan Druart from comment #8)
> (In reply to Victor Grousset/tuxayo from comment #7)
> > After changing the syspref, I must restart ZOOM to have a different number
> > of hits.
> > 
> > Is that expected?
> 
> No, please detail what you tried and the result.

Hum forget that, you detailed above.

Created attachment 107621 [details] [review]
Bug 25292: Flushing L1 on every Z3950 server search

This patch flushes the L1 cache on each Z3950 server search.

There is no need to flush it in init_handler (when the connection is
made), the L1 cache is not accessed (this behaviour goes against what I
expected).

This means each search will fetch values from the
L2 cache (ie memcached) and persist the L1 cache (in Perl
memory cache) until the result are returned (or longer?).

Test plan:
% perl misc/z3950_responder.pl --config-dir=/kohadevbox/koha/etc/z3950
% zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search x
ZOOM>search x

Between the 2 searches you can modify the value of SearchEngine, you
will notice that the number of hits is different

I am really confused, I am pretty sure the original patch was working when I wrote it but... reading the code it couldn't have worked.

This new patch should work accordingly to what is advertised in the commit message.

Tried to test this on Debian 9.13, but when I try to install Net::Z3950::SimpleServer it says "You have YAZ version 4.2.30; you need 4.2.51 or better. at Makefile.PL line 23", so I'm guessing I would need to have Debian 10.

(In reply to Magnus Enger from comment #12)
> Tried to test this on Debian 9.13, but when I try to install
> Net::Z3950::SimpleServer it says "You have YAZ version 4.2.30; you need
> 4.2.51 or better. at Makefile.PL line 23", so I'm guessing I would need to
> have Debian 10.

hi Magnus, have you tried using the version in the debian.org repo?

https://packages.debian.org/stretch/libnet-z3950-simpleserver-perl

Created attachment 114436 [details] [review]
Bug 25292: Flushing L1 on every Z3950 server search

This patch flushes the L1 cache on each Z3950 server search.

There is no need to flush it in init_handler (when the connection is
made), the L1 cache is not accessed (this behaviour goes against what I
expected).

This means each search will fetch values from the
L2 cache (ie memcached) and persist the L1 cache (in Perl
memory cache) until the result are returned (or longer?).

Test plan:
% perl misc/z3950_responder.pl --config-dir=/kohadevbox/koha/etc/z3950
% zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search x
ZOOM>search x

Between the 2 searches you can modify the value of SearchEngine, you
will notice that the number of hits is different

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Works well when you have both searchengine running.

Noticed that daemon fails when you switch to Elasticsearch without having it installed.
But that is for another bug.

Best regards ;)

Created attachment 114976 [details] [review]
Bug 25292: Flushing L1 on every Z3950 server search

This patch flushes the L1 cache on each Z3950 server search.

There is no need to flush it in init_handler (when the connection is
made), the L1 cache is not accessed (this behaviour goes against what I
expected).

This means each search will fetch values from the
L2 cache (ie memcached) and persist the L1 cache (in Perl
memory cache) until the result are returned (or longer?).

Test plan:
% perl misc/z3950_responder.pl --config-dir=/kohadevbox/koha/etc/z3950
% zoomsh
ZOOM>connect localhost:2100
ZOOM>set databaseName biblios
ZOOM>search x
ZOOM>search x

Between the 2 searches you can modify the value of SearchEngine, you
will notice that the number of hits is different

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Pushed to master for 21.05, thanks to everybody involved!

Pushed to 20.11.x for 20.11.02

Pushed to 20.05.x for 20.05.08

Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.