| Summary: | incorrect initialization parameters passed to Memoize::Memcached | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Galen Charlton <gmcharlt> |
| Component: | Architecture, internals, and plumbing | Assignee: | Galen Charlton <gmcharlt> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | jcamins, jonathan.druart, mtj, srdjan, veron |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
bug 10503: fix various issues with use of Memoize::Memcached
bug 10503: fix various issues with use of Memoize::Memcached [SIGNED-OFF] bug 10503: fix various issues with use of Memoize::Memcached |
||
Created attachment 19232 [details] [review] bug 10503: fix various issues with use of Memoize::Memcached Memoize::Memcached expects to be passed a hashref of connection parameters, which it in turn uses to initialize a Cache::Memcached object. However, commit 52afe06ddd started passing a Cache::Memcached object directly, not the required hashref. This happened to work by coincidence, but no longer works with recent versions of Memoize::Memcached packaged for Debian. This patch introduces a new C4::Context function, memcached_params(), that returns a hashref of connection parameters suitable for passing to the Cache::Memcached constructor. It also introduces a UNITCHECK block to C4::Context so that memcached_params() can be used during intialization of the module. This patch also corrects SQLHelper so that it no longer tries to use the deprecated (and removed) memcached_servers setting in koha-conf.xml; it now gets the memcached connection parameters from C4::Context->memcached_params(). To test: [1] This patch is most easily tested on Debian Wheezy, as version 0.03-2 of the libmemoize-memcached-perl package includes the fix for Debian bug 614868 that causes Koha's use of Memoize::Memcached to break. If you don't want to go so far as to install/upgrade to Wheezy, you can apply the following Debian patch to your copy of Memoize::Memcached to get the effect: http://patch-tracker.debian.org/patch/series/view/libmemoize-memcached-perl/0.03-2/universal.patch [2] Set up your Koha Apache configuration to point to a memcached server. [3] Restart memcached to start with a clear cache. [4] Do the following steps: - Create a new bib record. Note that it is sufficient to just open the editor; you don't need to save the bib. - Search for a patron record. [5] Use the memcdump command-line to get a list of keys in your memcached cache. You should *not* see keys that contain the strings GetMarcStructure, getTranslatedLanguages, or _get_columns. [6] Apply the patch, then go through step 4 again. [7] Use memcdump again. This time, you *should* see the keys mentioned in step 5. [8] Set the SessionStorage system preference to memcached. After logging in again, verify that your staff interface and OPAC sessions work without requiring that you log in for each and every page load. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Note to testers: After running through the test plan, this is what I got when I ran memcdump: $ memcdump --servers 127.0.0.1:11211 KOHAGMCismemcached KOHAGMC970239eca4adb0bb2bb8f4dc761a3023 KOHAGMCmemoize-getTranslatedLanguages-scalar-intranet KOHAGMCkohaconf KOHAGMCmemoize-_get_columns-scalar-borrowers KOHAGMCmemoize-GetMarcStructure-scalar-1 The key names will vary based on what you set MEMCACHED_NAMESPACE to. Created attachment 19236 [details] [review] bug 10503: fix various issues with use of Memoize::Memcached Memoize::Memcached expects to be passed a hashref of connection parameters, which it in turn uses to initialize a Cache::Memcached object. However, commit 52afe06ddd started passing a Cache::Memcached object directly, not the required hashref. This happened to work by coincidence, but no longer works with recent versions of Memoize::Memcached packaged for Debian. This patch introduces a new C4::Context function, memcached_params(), that returns a hashref of connection parameters suitable for passing to the Cache::Memcached constructor. It also introduces a UNITCHECK block to C4::Context so that memcached_params() can be used during intialization of the module. This patch also corrects SQLHelper so that it no longer tries to use the deprecated (and removed) memcached_servers setting in koha-conf.xml; it now gets the memcached connection parameters from C4::Context->memcached_params(). To test: [1] This patch is most easily tested on Debian Wheezy, as version 0.03-2 of the libmemoize-memcached-perl package includes the fix for Debian bug 614868 that causes Koha's use of Memoize::Memcached to break. If you don't want to go so far as to install/upgrade to Wheezy, you can apply the following Debian patch to your copy of Memoize::Memcached to get the effect: http://patch-tracker.debian.org/patch/series/view/libmemoize-memcached-perl/0.03-2/universal.patch [2] Set up your Koha Apache configuration to point to a memcached server. [3] Restart memcached to start with a clear cache. [4] Do the following steps: - Create a new bib record. Note that it is sufficient to just open the editor; you don't need to save the bib. - Search for a patron record. [5] Use the memcdump command-line to get a list of keys in your memcached cache. You should *not* see keys that contain the strings GetMarcStructure, getTranslatedLanguages, or _get_columns. [6] Apply the patch, then go through step 4 again. [7] Use memcdump again. This time, you *should* see the keys mentioned in step 5. [8] Set the SessionStorage system preference to memcached. After logging in again, verify that your staff interface and OPAC sessions work without requiring that you log in for each and every page load. Signed-off-by: Galen Charlton <gmc@esilibrary.com> I can see GetMarcStructure and getTranslatedLanguages, but not _get_columns (In reply to Srdjan Jankovic from comment #4) > I can see GetMarcStructure and getTranslatedLanguages, but not _get_columns I tried again and it works for me: $ memcdump --servers localhost:11211 KOHAismemcached KOHAmemoize-getTranslatedLanguages-scalar-intranet KOHAmemoize-getTranslatedLanguages-list-intranet KOHAkohaconf KOHAmemoize-_get_columns-scalar-borrowers Could you try again, and if it doesn't work, provide more details about your test environment? Created attachment 19735 [details] [review] [SIGNED-OFF] bug 10503: fix various issues with use of Memoize::Memcached Memoize::Memcached expects to be passed a hashref of connection parameters, which it in turn uses to initialize a Cache::Memcached object. However, commit 52afe06ddd started passing a Cache::Memcached object directly, not the required hashref. This happened to work by coincidence, but no longer works with recent versions of Memoize::Memcached packaged for Debian. This patch introduces a new C4::Context function, memcached_params(), that returns a hashref of connection parameters suitable for passing to the Cache::Memcached constructor. It also introduces a UNITCHECK block to C4::Context so that memcached_params() can be used during intialization of the module. This patch also corrects SQLHelper so that it no longer tries to use the deprecated (and removed) memcached_servers setting in koha-conf.xml; it now gets the memcached connection parameters from C4::Context->memcached_params(). To test: [1] This patch is most easily tested on Debian Wheezy, as version 0.03-2 of the libmemoize-memcached-perl package includes the fix for Debian bug 614868 that causes Koha's use of Memoize::Memcached to break. If you don't want to go so far as to install/upgrade to Wheezy, you can apply the following Debian patch to your copy of Memoize::Memcached to get the effect: http://patch-tracker.debian.org/patch/series/view/libmemoize-memcached-perl/0.03-2/universal.patch [2] Set up your Koha Apache configuration to point to a memcached server. [3] Restart memcached to start with a clear cache. [4] Do the following steps: - Create a new bib record. Note that it is sufficient to just open the editor; you don't need to save the bib. - Search for a patron record. [5] Use the memcdump command-line to get a list of keys in your memcached cache. You should *not* see keys that contain the strings GetMarcStructure, getTranslatedLanguages, or _get_columns. [6] Apply the patch, then go through step 4 again. [7] Use memcdump again. This time, you *should* see the keys mentioned in step 5. [8] Set the SessionStorage system preference to memcached. After logging in again, verify that your staff interface and OPAC sessions work without requiring that you log in for each and every page load. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Sorry, it is there. I'm going to see my optometrist today, that should help. Looks good to me, the patch works as expected on Debian Wheezy.
> This patch also corrects SQLHelper so that it no longer tries to
> use the deprecated (and removed) memcached_servers setting in koha-conf.xml;
Why do you say that?
it is still in used by Koha/Template/Plugin/Cache.pm and appears in debian/templates/koha-conf-site.xml.in.
Should we remove these occurrences too?
(In reply to Jonathan Druart from comment #8) > Looks good to me, the patch works as expected on Debian Wheezy. > > > This patch also corrects SQLHelper so that it no longer tries to > > use the deprecated (and removed) memcached_servers setting in koha-conf.xml; > > Why do you say that? > it is still in used by Koha/Template/Plugin/Cache.pm and appears in > debian/templates/koha-conf-site.xml.in. > > Should we remove these occurrences too? hmm, lets switch to 'in discussion'... (In reply to Jonathan Druart from comment #8) > Looks good to me, the patch works as expected on Debian Wheezy. > > > This patch also corrects SQLHelper so that it no longer tries to > > use the deprecated (and removed) memcached_servers setting in koha-conf.xml; > > Why do you say that? > it is still in used by Koha/Template/Plugin/Cache.pm and appears in > debian/templates/koha-conf-site.xml.in. > > Should we remove these occurrences too? That would be a good idea, yes. I think the 2 occurrences remaining should be replace. Marked as Failed QA. Still valid? No longer valid. |
Currently, Memoize::Memcached is initialized like this: eval { if (C4::Context->ismemcached) { require Memoize::Memcached; import Memoize::Memcached qw(memoize_memcached); memoize_memcached( 'GetMarcStructure', memcached => C4::Context->memcached); } }; C4::Context->memcached returns a Cache::Memcached object. However, that's not what Memoize::Memcached expects: This function also accepts a memcached option, which expects a hashref. This is de-referenced and passed directly into an internal function which sets up the memcached configuration for that function. This contents of this hashref are mostly options passed to Cache::Memcached, with a few exceptions. However, passing a Cache::Memcached happened to work, by coincidence, as Memoize::Memcached was calling UNIVERSAL::isa (in a deprecated fashion [0]) to check to see whether the passed argument is a HASH. Nowadays, though, since UNIVERSAL->import is deprecated, the version of Memoize::Memcached packaged for Debian Wheezy now uses ref() to check whether the argument is a hash [1]. That means that the coincidence no longer works, and consequently MARC frameworks don't get cached via memcached in Debian Wheezy, leading to a performance degradation. The solution is to pass Memoize::Memcached what it needs -- a hashref with the Cache::Memcached arguments -- and not a Cache::Memcached object. [0] https://rt.cpan.org/Public/Bug/Display.html?id=68170 [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=614868