Summary: | C4::Letters::getletter is not plack safe | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | abl, f.demians, julian.maurice, kyle, m.de.rooy |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 9452 | ||
Bug Blocks: | 7172, 16624 | ||
Attachments: |
Bug 16441: Do not use a package variable to cache C4::Letters::getletter
[SIGNED-OFF] Bug 16441: Do not use a package variable to cache C4::Letters::getletter Bug 16441: Fix Letters.t Bug 16441: Fix Letters.t Bug 16441: Do not use a package variable to cache C4::Letters::getletter Bug 16441: Fix Letters.t |
Description
Jonathan Druart
2016-05-04 18:43:30 UTC
Created attachment 51218 [details] [review] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense. Created attachment 51703 [details] [review] [SIGNED-OFF] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> (In reply to Jonathan Druart from comment #0) > It won't be a big deal to hit the DBMS to get a valid letter when needed. Hm. Not sure. getletter is called by GetPreparedLetter, SendCirculationAlert, etc. In loops ? -# FIXME: using our here means that a Plack server will need to be -# restarted fairly regularly when working with this routine. -# A better option would be to use Koha::Cache and use a cache -# that actually works in a persistent environment, but as a -# short-term fix, our will work. Why not use this opportunity to add caching? (In reply to Marcel de Rooy from comment #3) > (In reply to Jonathan Druart from comment #0) > > It won't be a big deal to hit the DBMS to get a valid letter when needed. > > Hm. Not sure. getletter is called by GetPreparedLetter, > SendCirculationAlert, etc. In loops ? > > -# FIXME: using our here means that a Plack server will need to be > -# restarted fairly regularly when working with this routine. > -# A better option would be to use Koha::Cache and use a cache > -# that actually works in a persistent environment, but as a > -# short-term fix, our will work. > > Why not use this opportunity to add caching? Because I have not found anywhere it's called many times. As I wrote previously, I am not sure it'd be interesting to cache it, but I may be wrong. If we decide to cache it, we need to make sure the values in cache are deleted when letters are updated/deleted. Anyway it will be a much bigger patch and more an enhancement than a bugfix. (In reply to Jonathan Druart from comment #4) > Because I have not found anywhere it's called many times. As I wrote > previously, I am not sure it'd be interesting to cache it, but I may be > wrong. > If we decide to cache it, we need to make sure the values in cache are > deleted when letters are updated/deleted. > Anyway it will be a much bigger patch and more an enhancement than a bugfix. Yes, it would be more work. But this patch affects performance. Note that I am not sure if this is actually a bugfix. The comment about regularly restarting plack is probably outdated. If we let each worker process only handle 50 requests, you do not need to restart plack very often. Since a new worker starts with an empty 'cache', you will not long see the old letter contents anymore. (In reply to Marcel de Rooy from comment #5) > The comment about regularly restarting plack is probably outdated. If we let > each worker process only handle 50 requests, you do not need to restart > plack very often. --max-requests Number of the requests to process per one worker process. Defaults to 1000. git grep max-requests: debian/scripts/koha-plack: STARMANOPTS="-M FindBin --max-requests 50 --workers 2 \ misc/plack/plackup.sh:# --max-requests 50 decreased from 1000 to keep memory usage sane misc/plack/plackup.sh:opt="$opt --server Starman -M FindBin --max-requests 50 --workers 4" Created attachment 52120 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. Working on tests for bug 16624, I have found that the tests were wrong, but we were not aware of that. Another point to remove this cache :) Created attachment 52130 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Planning to come back here soon :) Created attachment 52217 [details] [review] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Confirm that performance loss is just a millisecond or so per subsequent call of getletter. Created attachment 52218 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 16.11, thanks Jonathan! Pushed in 16.05. Will be in 16.05.01. Patches pushed to 3.22.x, will be in 3.22.8 |