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 - ?
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.
(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)?
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)
(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).
(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.
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
Increasing the severity as we need it in the next release.
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
Simple rebase
(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 ;)
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>
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.
(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?
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>
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>
(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.
Pushed to master for 17.05, thanks Jacek, Jonathan!
These patches have been pushed to 16.11.x and will be in 16.11.08.
Pushed to 16.05.x, for 16.05.13 release