Bug 16758 - Caching issues in scripts running in daemon mode
Summary: Caching issues in scripts running in daemon mode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
Depends on: 11998 16044 16190 17189
Blocks: 24761
  Show dependency treegraph
 
Reported: 2016-06-16 16:08 UTC by Jacek Ablewicz
Modified: 2020-03-02 02:27 UTC (History)
10 users (show)

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


Attachments
Bug 16758 - Caching issues in scripts running in daemon mode (2.43 KB, patch)
2016-09-14 13:50 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16758 - Caching issues in scripts running in daemon mode (2.47 KB, patch)
2017-03-07 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16758 - Caching issues in scripts running in daemon mode (2.50 KB, patch)
2017-04-04 12:53 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16758 - Caching issues in scripts running in daemon mode (2.56 KB, patch)
2017-05-08 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16758: Use the default cache instance (1.02 KB, patch)
2017-05-08 15:46 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 Jacek Ablewicz 2016-06-16 16:08:34 UTC
As L1 cache does not have expiration mechanism, scripts running in daemon mode (rebuild_zebra.pl -daemon, sip server ?, ...) would not be aware of any possible 
changes in the data being cached in Koha::Cache.

While adding expiration functionality to Koha::Cache would be pretty simple, I guess we don't really want to expire things by default after a given amount of seconds, that would not be good for overall performance; default expiration time for L2 = memcached is infinite (intentionally). Note that it's also infinite for L2 = Cache::Memory - probably not intentional, more likely that's because this code

    # Default cache time for memory is _always_ short unless it's specially
    # defined, to allow it to work reliably in a persistent environment.
    my $cache = Cache::Memory->new(
        'namespace'       => $self->{'namespace'},
        'default_expires' => "$self->{'timeout'} sec" || "10 sec",
    );

doesn't work as advertised. That might be a problem for plack + Cache::Memory setups - hopefully nobody uses such weird combos. Anyway, that belongs to the separate bug report.

I wonder what would be a best approach for solving this issue for perpetually running scripts? Maybe just adding a function to Koha::Cache which flushes L1 cache (but not more often then each 10 | 20 | 30 seconds), and call it in rebuild_zebra.pl etc. daemon loop  - ?
Comment 1 Tomás Cohen Arazi 2016-06-16 18:06:20 UTC
I think daemon scripts like this should have L1 disabled. If someone comes up with a proper caching solution for this, then have an enhancement bug. We should provide fixes for this on the stable releases, instead of backporting really complex architectural changes.
Comment 2 Jonathan Druart 2016-06-18 12:24:01 UTC
(In reply to Jacek Ablewicz from comment #0)
> I wonder what would be a best approach for solving this issue for
> perpetually running scripts? Maybe just adding a function to Koha::Cache
> which flushes L1 cache (but not more often then each 10 | 20 | 30 seconds),
> and call it in rebuild_zebra.pl etc. daemon loop  - ?

That sounds good to me.
What about flushing it before each run (in case of rebuild_rebra)?
Comment 3 Jonathan Druart 2016-06-18 12:26:57 UTC
Something like:

@@ -242,6 +243,7 @@ my $tester = XML::LibXML->new();
 if ($daemon_mode) {
     while (1) {
         # For incremental updates, skip the update if the updates are locked
+        Koha::Cache->flush_L1_cache;
         if (_flock($LockFH, LOCK_EX|LOCK_NB)) {
             do_one_pass() if ( zebraqueue_not_empty() );
             _flock($LockFH, LOCK_UN);

(+ use Koha::Cache)
Comment 4 Jacek Ablewicz 2016-09-06 07:09:59 UTC
(In reply to Jonathan Druart from comment #3)
> Something like:
> 
> @@ -242,6 +243,7 @@ my $tester = XML::LibXML->new();
>  if ($daemon_mode) {
>      while (1) {
>          # For incremental updates, skip the update if the updates are locked
> +        Koha::Cache->flush_L1_cache;
>          if (_flock($LockFH, LOCK_EX|LOCK_NB)) {
>              do_one_pass() if ( zebraqueue_not_empty() );
>              _flock($LockFH, LOCK_UN);
> 
> (+ use Koha::Cache)

That should work fine IMO, without significant performance hit (as long as L2 is memcached). Or maybe:

   - do_one_pass() if ( zebraqueue_not_empty() );
   + if ( zebraqueue_not_empty() ) {
   +     Koha::Cache->flush_L1_cache;
   +     do_one_pass();
   + }

would be a bit better (no need to clear the cache at each and every pass if there is nothing to reindex).
Comment 5 Jacek Ablewicz 2016-09-06 07:40:24 UTC
(In reply to Jacek Ablewicz from comment #4)

> That should work fine IMO, without significant performance hit (as long as
> L2 is memcached).

But if L2 = Cache::Memory, this will be effectively just a null op.

This might happen e.g. if the caching system is not configured quite consistently (due to MEMCACHED_* environment variables not exported by default for system & maintenance scripts). I'm not sure if it's still an issue for packaged debian installs or not (AFAIRC it was an issue at least in 3.22.* about 4 months ago).

BTW, I'm wondering what is an order of starting koha-common && memcached in debian packaged installs? If on server reboot indexer daemon gets started before memcached is up and running, it will default to Cache::Memory as well, even if the MEMCACHED_* variables are set up properly.
Comment 6 Jacek Ablewicz 2016-09-14 13:50:46 UTC
Created attachment 55564 [details] [review]
Bug 16758 - Caching issues in scripts running in daemon mode

As L1 cache does not have expiration mechanism, scripts running
in daemon mode (rebuild_zebra.pl -daemon, sip server ?, ...) would
not be aware of any possible changes in the data being cached
in upstream L2 cache.

This patch adds ->flush_L1_caches() call in rebuild_zebra.pl
inside daemon mode loop.

To test:

1) apply patch
2) ensure that rebuild_zebra.pl -daemon is still working properly,
without any noticeable performance degradation
3) stop memcached daemon and try to run rebuild_zebra.pl -daemon
again: there should be a warning emitted stating that the script
is running in daemon mode but without recommended caching system
Comment 7 Jonathan Druart 2016-10-18 15:55:11 UTC
Increasing the severity as we need it in the next release.
Comment 8 Marcel de Rooy 2017-03-07 09:44:26 UTC
Created attachment 60858 [details] [review]
Bug 16758 - Caching issues in scripts running in daemon mode

As L1 cache does not have expiration mechanism, scripts running
in daemon mode (rebuild_zebra.pl -daemon, sip server ?, ...) would
not be aware of any possible changes in the data being cached
in upstream L2 cache.

This patch adds ->flush_L1_caches() call in rebuild_zebra.pl
inside daemon mode loop.

To test:

1) apply patch
2) ensure that rebuild_zebra.pl -daemon is still working properly,
without any noticeable performance degradation
3) stop memcached daemon and try to run rebuild_zebra.pl -daemon
again: there should be a warning emitted stating that the script
is running in daemon mode but without recommended caching system
Comment 9 Marcel de Rooy 2017-03-07 09:44:42 UTC
Simple rebase
Comment 10 Marcel de Rooy 2017-03-07 11:31:43 UTC
(In reply to Jacek Ablewicz from comment #5)
> BTW, I'm wondering what is an order of starting koha-common && memcached in
> debian packaged installs? If on server reboot indexer daemon gets started
> before memcached is up and running, it will default to Cache::Memory as
> well, even if the MEMCACHED_* variables are set up properly.

In my rc5.d I have:
S01koha-common
S03memcached

So yes, koha-common (including indexer, sip) starts before memcached. I added a debug print in get_instance and saw a few lines with Cache::Memory in the logs when rebooting.

We probably should use update-rc.d to lower the priority of koha-common in the Debian package install.

Patch complexity is definitely not trivial ;)
Comment 11 Josef Moravec 2017-04-04 12:53:10 UTC
Created attachment 61820 [details] [review]
[SIGNED-OFF] Bug 16758 - Caching issues in scripts running in daemon mode

As L1 cache does not have expiration mechanism, scripts running
in daemon mode (rebuild_zebra.pl -daemon, sip server ?, ...) would
not be aware of any possible changes in the data being cached
in upstream L2 cache.

This patch adds ->flush_L1_caches() call in rebuild_zebra.pl
inside daemon mode loop.

To test:

1) apply patch
2) ensure that rebuild_zebra.pl -daemon is still working properly,
without any noticeable performance degradation
3) stop memcached daemon and try to run rebuild_zebra.pl -daemon
again: there should be a warning emitted stating that the script
is running in daemon mode but without recommended caching system

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Jonathan Druart 2017-04-12 16:30:04 UTC
Why do you get the "syspref" cache instance?
I guess there is no reason, I'd suggest to remove it and retrieve the default one.
Comment 13 Jonathan Druart 2017-05-02 16:30:29 UTC
(In reply to Jonathan Druart from comment #12)
> Why do you get the "syspref" cache instance?
> I guess there is no reason, I'd suggest to remove it and retrieve the
> default one.

Jacek,
It would be great to have this one into 17.05, can you answer please?
Comment 14 Jonathan Druart 2017-05-08 15:46:02 UTC
Created attachment 63247 [details] [review]
Bug 16758 - Caching issues in scripts running in daemon mode

As L1 cache does not have expiration mechanism, scripts running
in daemon mode (rebuild_zebra.pl -daemon, sip server ?, ...) would
not be aware of any possible changes in the data being cached
in upstream L2 cache.

This patch adds ->flush_L1_caches() call in rebuild_zebra.pl
inside daemon mode loop.

To test:

1) apply patch
2) ensure that rebuild_zebra.pl -daemon is still working properly,
without any noticeable performance degradation
3) stop memcached daemon and try to run rebuild_zebra.pl -daemon
again: there should be a warning emitted stating that the script
is running in daemon mode but without recommended caching system

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2017-05-08 15:46:07 UTC
Created attachment 63248 [details] [review]
Bug 16758: Use the default cache instance

I do not see a valid reason not to use the default one instead of the
syspref one.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jacek Ablewicz 2017-05-11 11:00:50 UTC
(In reply to Jonathan Druart from comment #12)
> Why do you get the "syspref" cache instance?
> I guess there is no reason, I'd suggest to remove it and retrieve the
> default one.

When rebuild_zebra.pl starts, it immediately initialises all 3 instances (config, syspref and default - in that order), so it probably doesn't matter much which one is used for checking if memcached is in use or not in use. Bug 18250 seems to be a bigger issue (possibility that zebra_daemon gets started before memcached is up and running). Overall, I don't think this is such a major issue for zebra daemon right now, because by a look of it, rebuild_zebra.pl uses very limited set of cached values, and predominantly (if not only) such kinds of values that are never changed in the production systems, like:

   syspref_cas* (due to BEGIN block in Auth.pm), syspref_marcflavour, syspref_includeseefrominsearches

It fetches repeatedly MarcSubfieldStructure-* from default cache instance, but I think it's mostly used for determining tag numbers (item tag number, biblio/biblioitem tag number), which don't change very often (if ever) in the production environments.
Comment 17 Kyle M Hall 2017-05-12 12:49:55 UTC
Pushed to master for 17.05, thanks Jacek, Jonathan!
Comment 18 Katrin Fischer 2017-05-14 09:49:11 UTC
These patches have been pushed to 16.11.x and will be in 16.11.08.
Comment 19 Mason James 2017-05-24 02:33:57 UTC
Pushed to 16.05.x, for 16.05.13 release