[Bug 16579] New: Use separate memached namespace for caching koha-conf.xml
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Bug ID: 16579 Summary: Use separate memached namespace for caching koha-conf.xml Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org The memcached configuration definitions were moved from koha-conf.xml to environment variables to enable caching of the conf file itself. This can cause numerous issues and difficulties making sure memcached is enabled for all cron scripts, for plack, etc. I propose we cache the koha conf file in a separate namespace with the caching still defined in environment variables, but move the memcached configuration for everything else back into the koha conf file itself. This would allow us to continue caching everything, but even if the cache for the koha-conf file is not configured for a given process, it would allow all other data to still be cached and used. This would greatly improve overall performance I think, especially for cron scripts that use the calendar for holiday handling. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jweaver@bywatersolutions.co | |m --- Comment #1 from Jesse Weaver <jweaver@bywatersolutions.com> --- I'd like to suggest that we either: a) drop the caching for the koha-conf.xml entirely (in my testing, this adds about .14 seconds to an average CGI load) or b) just hardcode the server/namespace for caching the koha-conf.xml and use the filename of the .xml as the cache key (kohaconf-/etc/koha/sites/SITE/koha-conf.xml). Using localhost:11211 will work for 99.9% of setups, and if memcached isn't running there, Koha will still work (just a tiny bit slower under CGI). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz, | |jonathan.druart@bugs.koha-c | |ommunity.org, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jesse Weaver from comment #1)
I'd like to suggest that we either:
a) drop the caching for the koha-conf.xml entirely (in my testing, this adds about .14 seconds to an average CGI load) or b) just hardcode the server/namespace for caching the koha-conf.xml and use the filename of the .xml as the cache key (kohaconf-/etc/koha/sites/SITE/koha-conf.xml). Using localhost:11211 will work for 99.9% of setups, and if memcached isn't running there, Koha will still work (just a tiny bit slower under CGI).
I think I'm for option a at this point. The cons seem to far outweigh the pros. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am not in favor of adding 0.15s when we are fighting to gain ms everywhere in the code, so I'd vote for b. (In reply to Kyle M Hall from comment #0)
This can cause numerous issues and difficulties making sure memcached is enabled for all cron scripts, for plack, etc.
Could you detail the issues and difficulties? Why is it hard to set the env var? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- But b is not ideal neither. You expect that memcached won't be use if the env var is not defined. We can imagine a sysop wanting to disable the cache (because a bug has been found). He does not necessarily want to stop memcached. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=11921 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #5 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- 140 miliseconds? Quite a lot - I know config file is currently parsed 3 times by an average script (for no apparent reason), but on my test setup, C4::Context->new() call only takes ~5.1 miliseconds. Memcache not enabled. And I have three Z39.50 servers defined in there. With Cache => 'memcopy' added to XMLin() in C4/Context.pm, its down to 92 microseconds per call. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #6 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- Either way, I have a feeling that implementing it (fetching memcache settings from Koha config) is not going to be easy. Fastmmap caching system is still broken, because it wasn't easy in that case (see Bug 16067 & Bug 11998 comment #114 +). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #7 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #3)
Why is it hard to set the env var?
Maybe it's not that hard, but it needs to be done in many places, and somehow it gets often forgotten. E.g., in 3.22.6 (or 3.22.7) installed from packages, memcached is enabled by default for CGI/plack, but memcached environment variables are not defined for shell / maintenance scripts. After installer/data/mysql/updatedatabase.pl DB update is done from command line, GUI is still prompting for an update, because it fetches (outdated) version preference from the memcache. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Use separate memached |Use separate memcached |namespace for caching |namespace for caching |koha-conf.xml |koha-conf.xml -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |6193 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6193 [Bug 6193] Use memcached cache koha-conf.xml configuration variables -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |WONTFIX --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- I think bug 11921 is a far more sane solution than this. Far fewer moving parts, less prone to regressions. Setting this to WONTFIX for the time being. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #9 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- To make things a bit more complicated, this separate cache is used not just for caching koha config in C4/Context; e.g. in C4/Languages.pm: if (C4::Context->ismemcached) { require Memoize::Memcached; import Memoize::Memcached qw(memoize_memcached); memoize_memcached('getTranslatedLanguages', memcached => memcached => C4::Context->memcached); memoize_memcached('getFrameworkLanguages', memcached => memcached => C4::Context->memcached); memoize_memcached('getAllLanguages', memcached => memcached => C4::Context->memcached); } Looks like an excellent material for a bunch of the brand new '... is not plack safe' bug reports ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16770 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #9)
To make things a bit more complicated, this separate cache is used not just for caching koha config in C4/Context; e.g. in C4/Languages.pm:
At least not in C4/Languages, this bunch of code is buggy. See bug 16770. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|WONTFIX |--- --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Maybe it's silly but I have another option: c) move the memcached config to a syspref and do not fetch this pref from the cache (like we do for OVERRIDE_SYSPREF_*) It will 1/ avoid to parse the koha conf file, 2/ keep the ability to cache it, 3/ add the ability to use the cache from anywhere and 4/ remove the need to export env vars. I have to admit it's not its place and the cache config should not be stored in the DB, but it looks like a tempting solution. Do I miss something? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #12 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #11)
c) move the memcached config to a syspref and do not fetch this pref from the cache (like we do for OVERRIDE_SYSPREF_*)
You mean, to fetch memcached setup directly from the database? But to do it you'll need to parse koha config file first, to obtain database connection config.
It will 1/ avoid to parse the koha conf file
I don't see how this step can be avoided in such scenario (?). 2nd problem with this scenario is that plack startup script will initialize DB connection (1st 'use C4::Context;' will trigger it) and without some extra care this single DB connection will then be shared between multiple plack workers. But this is not really a big problem, just a matter of adding e.g. { ## pre-init DBI / DBIx database connection my $schema = Koha::Database->new()->schema(); my $dbh = C4::Context->dbh; $dbh->do('SELECT * FROM systempreferences WHERE 1 = 0'); Koha::Config::SysPrefs->find('marcflavour'); ## trigger lazy-loaded modules compilation (conditional ## requires etc.) in Template::*, CGI and Koha::Dateutils # Template::Config->preload(); # CGI->compile(':all'); ## initializes locale and timezone related stuff # dt_from_string(); ## disconnect from the database, otherwise the single database ## connection will be shared between multiple plack workers $schema->storage()->disconnect(); } at the end of the startup script. Actually, this may have some extra adventages as it will allow safe preloading of many more modules at startup (good for performance and really good for memory footprint, due to copy-on-write), including some problematic ones like e.g. C4/Auth.pm (which calls ->preference() in BEGIN block). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #12)
(In reply to Jonathan Druart from comment #11)
c) move the memcached config to a syspref and do not fetch this pref from the cache (like we do for OVERRIDE_SYSPREF_*)
You mean, to fetch memcached setup directly from the database? But to do it you'll need to parse koha config file first, to obtain database connection config.
It will 1/ avoid to parse the koha conf file
I don't see how this step can be avoided in such scenario (?).
No of course we will have to parse it :) I meant we will just need to parse it once. Once parsed, we could cache it and avoid to parse it for each request. But actually it does not make sense to move it to syspref, there is no additional advantage compared to koha-conf (it's even worst, we need the dbh). I have worked on a proposal, patches are coming -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52705 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52705&action=edit Bug XXXXX_1: Fix support of several memcached servers There is a bug in the initialisation of memcached, the server string is not correctly parsed. If several memcached servers are defined, they are separated by commas and so the string should be split accordingly Test plan: 1/ Set MEMCACHED_SERVERS='localhost:11211,localhost:11222' 2/ Set DEBUG=1 3/ Reload plack, and check the log => Without this patch, you see Memcached server settings: localhost:11211,localhost:11222 with koha Selected caching system: Cache::Memory=HASH(0x9de2034) i.e. Memcached has not been configured correctly and the default caching system is used (Cache::Memory) => With this patch, you should see Memcached server settings: localhost:11211, localhost:11222 with koha Selected caching system: Cache::Memcached::Fast=SCALAR(0xa2a0c54) i.e. Memcached has been configured correctly \o/ Note that the cache_servers attribute is never set and it not used. It's better to remove it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52706&action=edit Bug XXXXX_2: Add the ability to define several namespaces - tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52707 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52707&action=edit Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches We need to define take into account several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52708 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52708&action=edit Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52709 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52709&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf MEMCACHED_SERVERS and MEMCACHED_NAMESPACE should be moved back to koha-conf instead of an ENV var to let command line scripts use the cache mechanism. TODO: - Add the entries to koha-conf.xml - Remove occurrences of MEMCACHED_SERVERS in package, apache and plack files -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52710 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52710&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config Move C4::Context->read_config_file to Koha::Config That permits to reuse it from Koha::Cache without needing C4::Context (and introduce a circular deps). TODO: Add decent POD to Koha::Config -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 52711 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52711&action=edit Bug XXXXX_3: Update remaining occurrences of ->ismemcached TODO: Would be better to provide a better way to know if memcache is correctly set -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Could someone take a look at these patches? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Patch doesn't apply --- Comment #22 from Chris Cormack <chris@bigballofwax.co.nz> --- Applying: Bug XXXXX_2: Add the ability to define several namespaces - tests fatal: sha1 information is lacking or useless (t/Cache.t). error: could not build fake ancestor Patch failed at 0001 Bug XXXXX_2: Add the ability to define several namespaces - tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |16166 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Chris Cormack from comment #22)
Applying: Bug XXXXX_2: Add the ability to define several namespaces - tests fatal: sha1 information is lacking or useless (t/Cache.t). error: could not build fake ancestor Patch failed at 0001 Bug XXXXX_2: Add the ability to define several namespaces - tests
Sorry, I forgot to add the dependent bug. It depends on bug 16166. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16166 [Bug 16166] Improve L1 cache performance and safety -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #24 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #21)
Could someone take a look at these patches?
Overall they look (mostly) fine to me, or at least a move in the right direction[s]. They are a bit hard to digest though, lots of collateral fixes included - if some of them could be (?) moved to the separate reports, signing off and Q&A will probably have a much better chance to occur in foreseeable future ;). Not sure if I fully understand a concept of namespaces / sub-namespaces introduced with this patch set (and what are they needed for exactly, especially in such particular form). E.g. L1 storage hash still remains a shared singleton (sort of) - moving L1 storage to the cache objects instead (and ideally tying those objects to the Koha contexts in 1:1 relationship) seems to be much less headache-prone long term solution IMO. By the look of it, in regards of cache architectural changes, there will be some overlap with Bug 15562, which in parts does somehow similar things with the cache (and a lot of other interesting things as well, but it's huge and I don't really understand ~half of it). Are there any chances of coordinated effort of some sort, so they both don't clash too badly with each other? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15562 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #25 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (In reply to Jacek Ablewicz from comment #24)
By the look of it, in regards of cache architectural changes, there will be some overlap with Bug 15562, which in parts does somehow similar things with the cache (and a lot of other interesting things as well, but it's huge and I don't really understand ~half of it). Are there any chances of coordinated effort of some sort, so they both don't clash too badly with each other?
That's terrible. Next time I rebase I'll try to make it more readable. Apologies. Changes presented here are almost identical to the changes 15562, except that they are a bit better: * I fell short of introducing Koha::Config which I should have (I chickened out) * Koha::Caches - maybe we need to mandate the namespace when instantiating Koha::Cache in order for this to be more robust? At any rate, I did my patch in order to make plack multi-host going, and to be brutally honest I was expecting things in the Cache/Context land to change a lot by the time my patch gets some love. The issue is very complex, so it is probably good to have independent efforts at early stages. Helps with discovering unforeseen situations. One thing that might be worth thinking about is that the very same problem exists with the database connections. So maybe some über class, Koha::Storage or something that would have a slot for everything we use, and then Koha::Storages collection keyed by the namespace? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hi Jacek and Srdjan! Thanks for taking a look and sorry for the delay to get back to you. (In reply to Jacek Ablewicz from comment #24)
(In reply to Jonathan Druart from comment #21)
Could someone take a look at these patches?
Overall they look (mostly) fine to me, or at least a move in the right direction[s]. They are a bit hard to digest though, lots of collateral fixes included - if some of them could be (?) moved to the separate reports, signing off and Q&A will probably have a much better chance to occur in foreseeable future ;).
That's why I have prefixed the commit messages with Bug XXXXX_1|2|3. Patches with "Bug XXXXX_1" should go on their own bug report, same with _2 and _3. I plan to move them, but I wanted to know if the different changes were appreciated before splitting them.
Not sure if I fully understand a concept of namespaces / sub-namespaces introduced with this patch set (and what are they needed for exactly, especially in such particular form). E.g. L1 storage hash still remains a shared singleton (sort of) - moving L1 storage to the cache objects instead (and ideally tying those objects to the Koha contexts in 1:1 relationship) seems to be much less headache-prone long term solution IMO.
Yes indeed, it was the easy and lazy approach. My first idea was to manage it from Koha::Caches which makes more sense, but it was not so easy. I'd be happy to improve that if you have a good idea on how to do it. (In reply to Srdjan Jankovic from comment #25)
(In reply to Jacek Ablewicz from comment #24)
By the look of it, in regards of cache architectural changes, there will be some overlap with Bug 15562, which in parts does somehow similar things with the cache (and a lot of other interesting things as well, but it's huge and I don't really understand ~half of it). Are there any chances of coordinated effort of some sort, so they both don't clash too badly with each other?
That's terrible. Next time I rebase I'll try to make it more readable. Apologies.
Changes presented here are almost identical to the changes 15562, except that they are a bit better: * I fell short of introducing Koha::Config which I should have (I chickened out) * Koha::Caches - maybe we need to mandate the namespace when instantiating Koha::Cache in order for this to be more robust?
Does that mean you would agree to let these patches make his own way then rebase yours? :)
One thing that might be worth thinking about is that the very same problem exists with the database connections. So maybe some über class, Koha::Storage or something that would have a slot for everything we use, and then Koha::Storages collection keyed by the namespace?
I think we are dealing pretty well with our cache improvements: we improve it a lot without breaking anything (so far!), using small steps. So I'd suggest to continue like that and keep in mind these Koha::Storage[s] classes What's next? :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52705|0 |1 is obsolete| | --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53945&action=edit Bug XXXXX_1: Fix support of several memcached servers There is a bug in the initialisation of memcached, the server string is not correctly parsed. If several memcached servers are defined, they are separated by commas and so the string should be split accordingly Test plan: 1/ Set MEMCACHED_SERVERS='localhost:11211,localhost:11222' 2/ Set DEBUG=1 3/ Reload plack, and check the log => Without this patch, you see Memcached server settings: localhost:11211,localhost:11222 with koha Selected caching system: Cache::Memory=HASH(0x9de2034) i.e. Memcached has not been configured correctly and the default caching system is used (Cache::Memory) => With this patch, you should see Memcached server settings: localhost:11211, localhost:11222 with koha Selected caching system: Cache::Memcached::Fast=SCALAR(0xa2a0c54) i.e. Memcached has been configured correctly \o/ Note that the cache_servers attribute is never set and it not used. It's better to remove it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52706|0 |1 is obsolete| | --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53946 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53946&action=edit Bug XXXXX_2: Add the ability to define several namespaces - tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52707|0 |1 is obsolete| | --- Comment #29 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53947&action=edit Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches We need to define take into account several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52708|0 |1 is obsolete| | --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53948 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53948&action=edit Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52709|0 |1 is obsolete| | --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53949 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53949&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf MEMCACHED_SERVERS and MEMCACHED_NAMESPACE should be moved back to koha-conf instead of an ENV var to let command line scripts use the cache mechanism. TODO: - Add the entries to koha-conf.xml - Remove occurrences of MEMCACHED_SERVERS in package, apache and plack files -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52710|0 |1 is obsolete| | --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53950&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config Move C4::Context->read_config_file to Koha::Config That permits to reuse it from Koha::Cache without needing C4::Context (and introduce a circular deps). TODO: Add decent POD to Koha::Config -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52711|0 |1 is obsolete| | --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53951 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53951&action=edit Bug XXXXX_3: Update remaining occurrences of ->ismemcached TODO: Would be better to provide a better way to know if memcache is correctly set -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53947|0 |1 is obsolete| | --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53952&action=edit Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches We need to define take into account several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53948|0 |1 is obsolete| | --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53953 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53953&action=edit Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53949|0 |1 is obsolete| | --- Comment #36 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53954&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf MEMCACHED_SERVERS and MEMCACHED_NAMESPACE should be moved back to koha-conf instead of an ENV var to let command line scripts use the cache mechanism. TODO: - Add the entries to koha-conf.xml - Remove occurrences of MEMCACHED_SERVERS in package, apache and plack files -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53950|0 |1 is obsolete| | --- Comment #37 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53955&action=edit Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config Move C4::Context->read_config_file to Koha::Config That permits to reuse it from Koha::Cache without needing C4::Context (and introduce a circular deps). TODO: Add decent POD to Koha::Config -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53951|0 |1 is obsolete| | --- Comment #38 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 53956 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53956&action=edit Bug XXXXX_3: Update remaining occurrences of ->ismemcached TODO: Would be better to provide a better way to know if memcache is correctly set -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #39 from Srdjan Jankovic <srdjan@catalyst.net.nz> ---
(In reply to Srdjan Jankovic from comment #25) Does that mean you would agree to let these patches make his own way then rebase yours? :)
Absolutely. It affects only one of my patches, and if lucky enough I may even be able to throw it away. It is a pre-patch anyway, to prepare the grounds for the real issue. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #40 from Nick Clemens <nick@bywatersolutions.com> --- I haven't fully tested, but initial read through and applying looks good. This makes it much easier to get memcached functioning with plack and is a good improvement -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #41 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Nick Clemens from comment #40)
I haven't fully tested, but initial read through and applying looks good. This makes it much easier to get memcached functioning with plack and is a good improvement
I agree! This is a huge improvement and greatly simplifies the setup. Jacek, what do you think? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #42 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Kyle M Hall from comment #41)
(In reply to Nick Clemens from comment #40)
I haven't fully tested, but initial read through and applying looks good. This makes it much easier to get memcached functioning with plack and is a good improvement
I agree! This is a huge improvement and greatly simplifies the setup.
Jacek, what do you think?
I think generally it's good, and certainly good enough for the 'promotion' from "In discussion" to "Needs sign-off". I have some doubts re/ certain parts (mostly about an introduction of that 'namespace' thingy to the Koha caching system - not entirely sure if it's strictly necessary, and - in case it is - if doing it in this particular way is the best way possible). But it's more the matter of opinion / personal preferences (and/or my lack of understanding why it's needed to be done, and/or why in that way) than any real concerns. So far I haven't spotted any regressions in this patch set - looks like there may be some (= most likely very-very slight) performance hits involved for the caching system, because of the 'namespaces' introduction, but IMO sorting out inconsistencies of the memcached config / setups problems in the systemic way is _much_ more important then that. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17188 --- Comment #43 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Bug XXXXX_1: Fix support of several memcached servers moved to bug 17188. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17188 [Bug 17188] Koha does not support several memcached servers -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17189 --- Comment #44 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Bug XXXXX_2: Add the ability to define several namespaces - tests Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences Moved to bug 17189. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17189 [Bug 17189] Add the ability to define several memcached namespaces -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |11921 --- Comment #45 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Bug XXXXX_3: Restore memcached infos to koha-conf Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config Bug XXXXX_3: Update remaining occurrences of ->ismemcached Moved to bug 11921 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11921 [Bug 11921] memcached configuration should be moved back to koha-conf.xml -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=11921 | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53945|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53946|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53952|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53953|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53954|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53955|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53956|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 --- Comment #46 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #42)
(In reply to Kyle M Hall from comment #41)
(In reply to Nick Clemens from comment #40)
I haven't fully tested, but initial read through and applying looks good. This makes it much easier to get memcached functioning with plack and is a good improvement
I agree! This is a huge improvement and greatly simplifies the setup.
Jacek, what do you think?
I think generally it's good, and certainly good enough for the 'promotion' from "In discussion" to "Needs sign-off". I have some doubts re/ certain parts (mostly about an introduction of that 'namespace' thingy to the Koha caching system - not entirely sure if it's strictly necessary, and - in case it is - if doing it in this particular way is the best way possible).
I agree, that's why I have added to FIXME in the commit message since the first submission :) FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache All the patches have been splited into 3 bug reports. Letting this one for the general discussion about these patches. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Use separate memcached |[OMNIBUS] Use separate |namespace for caching |memcached namespace for |koha-conf.xml |caching koha-conf.xml -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=17224 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Bug 16579 depends on bug 17188, which changed state. Bug 17188 Summary: Koha does not support several memcached servers https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17188 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |RESOLVED Resolution|--- |INVALID -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16579 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|In Discussion |RESOLVED --- Comment #47 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- The 3 bugs 11921, 16166 and 17189 have been pushed to master. Closing this one. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org