[Bug 29623] New: Cache circulation rules
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Bug ID: 29623 Summary: Cache circulation rules Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org We could cache the circulation rules (in memory/L1) to speed up areas where they are fetched several times (in a loop). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128180 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128180&action=edit Bug 29623: Cache circulation rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |29537 Status|ASSIGNED |In Discussion Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29537 [Bug 29537] Simplify auto-renewal code in CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29474 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Nick, that would be helpful if you could run the same tests as you did on bug 29474 comment 9, to see if we can gain more caching the circ rules in L1. This patch is absolutely not ready to be integrated into master but I would like to get some opinions about it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128180|0 |1 is obsolete| | --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128576 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128576&action=edit Bug 29623: Cache circulation rules Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Testing on top of 29703 this improves the performance on request.pl by about 1.5 seconds with 100 items, which is pretty decent -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Could I get a test plan for this please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Also, patch doesn't apply on top of dependency patch: 128576 - Bug 29623: Cache circulation rules Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 29623: Cache circulation rules error: sha1 information is lacking or useless (C4/Circulation.pm). error: could not build fake ancestor Patch failed at 0001 Bug 29623: Cache circulation rules hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-29623-Cache-circulation-rules-SDQmaq.patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128576|0 |1 is obsolete| | --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 134209 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134209&action=edit Bug 29623: Cache circulation rules Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Katrin Fischer from comment #5)
Could I get a test plan for this please?
Confirm that the rule is taken into account where get_effective_rules is used (hardduedate for instance). And that a new value is used when modified in the circ matrix (ie. the cache is flushed when a rule is updated). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Silly question... why do this just at the request caching level? I had a bit of a deep dive into our caching strategies yesterday to get myself back up to speed... Is it slower to fetch a key from Memcached once (to populate the L1 per request) than it is to fetch it from MySQL (to populate the same L1 per request)?.. or am I totally misunderstanding our caching code? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #9)
Silly question... why do this just at the request caching level?
Where else? Sorry I am not sure I understand your question.
I had a bit of a deep dive into our caching strategies yesterday to get myself back up to speed...
Is it slower to fetch a key from Memcached once (to populate the L1 per request) than it is to fetch it from MySQL (to populate the same L1 per request)?.. or am I totally misunderstanding our caching code?
Where do you read that in the code? I don't have numbers but yes, fetching something simple from MySQL may take the same time as fetching from Memcached. That must be confirmed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 134209 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134209 Bug 29623: Cache circulation rules Review of attachment 134209: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=29623&attachment=134209) ----------------------------------------------------------------- ::: Koha/CirculationRules.pm @@ +379,5 @@
+ my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + my $cache_key = sprintf "CircRules:%s:%s:%s:%s", $rule_name // q{}, + $categorycode // q{}, $branchcode // q{}, $itemtype // q{}; + + Koha::Cache::Memory::Lite->flush();
It looks like the $memory_cache and $cache_key variables are initialised but unused? @@ +410,4 @@
push( @$rule_objects, $rule_object ); }
+ Koha::Cache::Memory::Lite->flush();
Is this necessary since the cache should be cleared at the end of the HTTP request? Or is this to provide for cases where the method is called outside a HTTP context? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #9)
Silly question... why do this just at the request caching level?
I had a bit of a deep dive into our caching strategies yesterday to get myself back up to speed...
Is it slower to fetch a key from Memcached once (to populate the L1 per request) than it is to fetch it from MySQL (to populate the same L1 per request)?.. or am I totally misunderstanding our caching code?
The TCP speed is probably comparable although the MySQL is going to be doing some disk I/O while Memcached is all in memory so in theory Memcached should be faster, but the difference might be imperceptible with a low load and good disk*? *I have a different app that does a lot of I/O on very bad disks and even doing simple lookups can be hard when the disks are busy with other tasks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to David Cook from comment #11)
Comment on attachment 134209 [details] [review] Bug 29623: Cache circulation rules
Review of attachment 134209 [details] [review]: -----------------------------------------------------------------
::: Koha/CirculationRules.pm @@ +379,5 @@
+ my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + my $cache_key = sprintf "CircRules:%s:%s:%s:%s", $rule_name // q{}, + $categorycode // q{}, $branchcode // q{}, $itemtype // q{}; + + Koha::Cache::Memory::Lite->flush();
It looks like the $memory_cache and $cache_key variables are initialised but unused?
Looks like a leftover. I think I first put the code there then moved it into the other method.
@@ +410,4 @@
push( @$rule_objects, $rule_object ); }
+ Koha::Cache::Memory::Lite->flush();
Is this necessary since the cache should be cleared at the end of the HTTP request? Or is this to provide for cases where the method is called outside a HTTP context?
Hum, that's bad indeed. I think we need to flush (as the cache contains old values now), or update the cache's values, but we shouldn't flush the whole memory cache. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 135463 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135463&action=edit Bug 29623: Don't flush the whole L1 cache We shouldn't flush L1 cache completely, only the values related to the circulation rules. It is not correct to update the value of the value we are currently setting (because of the inheritance concept of the circ rules). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I think this is the correct approach, 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=29623 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Rephrasing my question.. why the choice to use Koha::Cache::Memory::Lite instead of just Koha::Cache ? I don't think the approach is bad at all, just trying to understand the decisions made. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #16)
Rephrasing my question.. why the choice to use Koha::Cache::Memory::Lite instead of just Koha::Cache ? I don't think the approach is bad at all, just trying to understand the decisions made.
Because of the flush :) We cannot flush using regex, so I had to flush the whole L1. See the last patch :) The last patch does it now, but only for L1. I guess I also wanted to prevent bad values to be kept in cache, but... we shouldn't be afraid of that, right? We certainly could improve this idea, but having it as simple as it is now seems good to me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #18 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #15)
I think this is the correct approach, what do you think?
I'm not 100% sure. I find that the code in ./admin/smart-rules.pl and Koha/CirculationsRules.pm is very messy. If we clear the cache after each set_rule() call, we'll be calling the function ~38 times (so 38 loops) for each add/update/delete in ./admin/smart-rules.pl. We should only need to call it 1 time for each CRUD operation. In terms of core code, it looks like Koha::CirculationRules->set_rule only gets called in Koha/CirculationRules.pm and once accidentally in ./admin/smart-rules.pl (it should use set_rules instead). We do call set_rule() a lot in the unit tests though. I suppose occasional inefficient cache setting/clearing in ./admin/smart-rules.pl is worth a performance speed up during frequent transactional work. In fact, since we're using the L1 cache and the L1 cache is flushed at the end of every HTTP/SIP request, clearing the cache (in ./admin/smart-rules.pl) is actually probably totally unnecessary for production purposes. The only time caching of circulation rules would be a potential problem is in the unit tests. So I'd say the cache clearing is messy but it's good enough. (Side note: the circulation_rules table has an "id" column, so we should be using that for for distinct update_rules/delete_rules operations.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #15)
I think this is the correct approach, what do you think?
Since we are clearing the cache via set_rule, there's probably no harm in using the L2 cache via Koha::Cache. Nick has already signed off this version though, and I think this one is good enough for now at least. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- One last thing... we might want to rename this issue to be "Cache effective circulation rules" because that would be more accurate than "Cache circulation rules". We're not just saving a database call by caching. We're saving a database call that has embedded logic in it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to David Cook from comment #18)
(In reply to Jonathan Druart from comment #15)
I think this is the correct approach, what do you think?
I'm not 100% sure.
I find that the code in ./admin/smart-rules.pl and Koha/CirculationsRules.pm is very messy.
Let's say all our code is messy and so we don't need to tell it every week, deal?
If we clear the cache after each set_rule() call, we'll be calling the function ~38 times (so 38 loops) for each add/update/delete in ./admin/smart-rules.pl. We should only need to call it 1 time for each CRUD operation.
We could add a flag to set_rule to not clear the cache, and clear it from set_rules. Would that work for you?
In fact, since we're using the L1 cache and the L1 cache is flushed at the end of every HTTP/SIP request, clearing the cache (in ./admin/smart-rules.pl) is actually probably totally unnecessary for production purposes.
Seems a good practices to invalided the cache when the value might have changed. That will prevent bugs in the future. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Cache circulation rules |Cache effective circulation | |rules --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to David Cook from comment #20)
One last thing... we might want to rename this issue to be "Cache effective circulation rules" because that would be more accurate than "Cache circulation rules".
Done. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #21)
Let's say all our code is messy and so we don't need to tell it every week, deal?
Hehe deal.
We could add a flag to set_rule to not clear the cache, and clear it from set_rules. Would that work for you?
I thought about that but the existing patches are already signed off, so it's probably not a necessary optimization at this point.
In fact, since we're using the L1 cache and the L1 cache is flushed at the end of every HTTP/SIP request, clearing the cache (in ./admin/smart-rules.pl) is actually probably totally unnecessary for production purposes.
Seems a good practices to invalided the cache when the value might have changed. That will prevent bugs in the future.
Agreed. -- Overall, I don't see any blocks/fails for this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I think we can probably call this one PQA now.. but I'd love to see a follow-up but to convert to Koha::Cache as we've pretty much done all the work here to make sure we're resetting the caches as required on changes and caching for longer makes sense. I'm also interested in David's suggestion about using ID's for the cache key, but I've not investigated the code to validate that myself. David, did you fancy adding your QA stamp? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #25 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #24)
David, did you fancy adding your QA stamp?
I don't think I'm officially on the QA team anymore 😅 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #24)
I think we can probably call this one PQA now.. but I'd love to see a follow-up but to convert to Koha::Cache as we've pretty much done all the work here to make sure we're resetting the caches as required on changes and caching for longer makes sense.
Yes, maybe. However I am expecting several adjustments in test files as we are deleting all the entries from circulation_rules in some tests.
I'm also interested in David's suggestion about using ID's for the cache key, but I've not investigated the code to validate that myself.
I don't think it's possible, get_effective_rule_value does not have the ID. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #26)
I'm also interested in David's suggestion about using ID's for the cache key, but I've not investigated the code to validate that myself.
I don't think it's possible, get_effective_rule_value does not have the ID.
Yeah, there's quite a bit of refactoring to do on the CRUD before we could use the ID for the caching. My earlier suggestion was a "someday" suggestion heh. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #28 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- IMO It's not about refactoring, it's just not possible. get_effective_rule will still need to get (category, library, itemtype) in parameter. If the caller has the rule's id, it does not need to ask for the *effective* rule :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134209|0 |1 is obsolete| | --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135916 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135916&action=edit Bug 29623: Cache circulation rules Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135463|0 |1 is obsolete| | --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135917 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135917&action=edit Bug 29623: Don't flush the whole L1 cache We shouldn't flush L1 cache completely, only the values related to the circulation rules. It is not correct to update the value of the value we are currently setting (because of the inheritance concept of the circ rules). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135918 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135918&action=edit Bug 29623: (QA follow-up) Add POD to Koha::Cache::Lite Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135919&action=edit Bug 29623: (QA follow-up) Add POD to Koha::CirculationRules Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- OK, all points raised in the discussion have been agreed upon.. Happy to PQA (with the POD follow-ups to squash QA script noise). Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #34 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #28)
IMO It's not about refactoring, it's just not possible. get_effective_rule will still need to get (category, library, itemtype) in parameter. If the caller has the rule's id, it does not need to ask for the *effective* rule :)
Agreed.. now I've delved into the code that's pretty obvious. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #35 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #28)
IMO It's not about refactoring, it's just not possible. get_effective_rule will still need to get (category, library, itemtype) in parameter. If the caller has the rule's id, it does not need to ask for the *effective* rule :)
I must've misworded my earlier comment. Originally, I meant refactoring to use the ID for doing database updates/deletions. Wasn't really related to the caching. Not sure how that got dragged into it. Totally separate thing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to David Cook from comment #12)
(In reply to Martin Renvoize from comment #9)
Silly question... why do this just at the request caching level?
I had a bit of a deep dive into our caching strategies yesterday to get myself back up to speed...
Is it slower to fetch a key from Memcached once (to populate the L1 per request) than it is to fetch it from MySQL (to populate the same L1 per request)?.. or am I totally misunderstanding our caching code?
The TCP speed is probably comparable although the MySQL is going to be doing some disk I/O while Memcached is all in memory so in theory Memcached should be faster, but the difference might be imperceptible with a low load and good disk*?
MySQL will be doing disk I/O on the first request for the same thing. I'm pretty sure the caching strategy in MySQL is far better than ours and memcached. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #37 from Tomás Cohen Arazi <tomascohen@gmail.com> --- The patchset tweaks Circulation.t to explicitly flush the cached rules in select places so I ran: $ git grep --name-only 'Koha::CirculationRule' t/ | xargs prove All good \o/ but keep in mind that we might need to write something in the guidelines for tests tweaking rules. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #38 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #39 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi from comment #36)
MySQL will be doing disk I/O on the first request for the same thing. I'm pretty sure the caching strategy in MySQL is far better than ours and memcached.
While I know what you mean about the disk caching, I'm not sure what you mean about it being better than ours. Caching a rule once per HTTP request makes more sense to me than fetching it X times over the course of that HTTP request. That said, it all depends on the volume. When working with hundreds, thousands, and millions, even fractional second lags add up. (I've had a lot of fun with that over the years just in terms of the difference in TCP lag of connecting to local TCP sockets vs remote TCP sockets.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Bug 29623 depends on bug 29537, which changed state. Bug 29537 Summary: Simplify auto-renewal code in CanBookBeRenewed https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29537 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to master |Needs documenting --- Comment #40 from Lucas Gass <lucas@bywatersolutions.com> --- Enhancement will not be backported to 22.05.x series -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #41 from Nick Clemens <nick@bywatersolutions.com> --- Jonathan, would you say this is eligible for backport? More performance than enhancement -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 --- Comment #42 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Nick Clemens from comment #41)
Jonathan, would you say this is eligible for backport? More performance than enhancement
We could backport, but I would wait a couple of months to let us time to catch potential problems. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Documentation| |Martin Renvoize contact| | Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #43 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Internal improvements, no documentation change required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32092 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #44 from Fridolin Somers <fridolin.somers@biblibre.com> --- *** Bug 26393 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |32092 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32092 [Bug 32092] Improve circulation rules cache utilization -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29623 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32092 | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org