[Bug 28173] New: Add plugin hooks object_store_pre and object_store_post
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Bug ID: 28173 Summary: Add plugin hooks object_store_pre and object_store_post Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Plugin architecture Assignee: julian.maurice@biblibre.com Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 119872 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119872&action=edit Bug 28173: Add plugin hooks object_store_pre and object_store_post These two hooks are called in Koha::Object::store, which means any object stored in the database will trigger these hooks. object_store_pre is called just before `update_or_insert`, allowing to modify the object before sending the INSERT/UPDATE query object_store_post is called just after `update_or_insert`, so it can be used for any actions that require to have the final object, like indexing, logging, ... Test plan: 1. prove t/db_dependent/Koha/Plugins/Object_hooks.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #2 from Fridolin Somers <fridolin.somers@biblibre.com> --- I like that. Just for hook name. In https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks we see existing hooks are calles after_xxx : after_biblio_action after_circ_action So i think whe should call this hooks : before_object_store after_object_store -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #3 from Fridolin Somers <fridolin.somers@biblibre.com> --- Oh crap. We found a big performance issue with this hook. Each store() call is checking in DB the plugin version and if it is enabled. Particulary when DB is distant this is bad for performance. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #4 from Fridolin Somers <fridolin.somers@biblibre.com> --- This hook is used by Biblibre for action on record before saving. I suggest we change to implement hooks 'before_authority_action' and 'before_biblio_action'. With record as param. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #5 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Fridolin Somers from comment #3)
Oh crap.
We found a big performance issue with this hook.
Each store() call is checking in DB the plugin version and if it is enabled.
Particulary when DB is distant this is bad for performance.
The problem is not this particular hook. It's Koha::Plugins->call that is too slow. I tried making it faster with bug 29672. Please test and let me know what you think -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29672 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #6 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Julian Maurice from comment #5)
The problem is not this particular hook. It's Koha::Plugins->call that is too slow. I tried making it faster with bug 29672. Please test and let me know what you think
I agree with you. Thanks a lot for the perf patch, looks great -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #7 from David Cook <dcook@prosentient.com.au> --- When I saw the hook names, I was worried about performance, and I can see that worry was well founded. I'm curious if bug 29672 will be enough. These hooks are pretty broad. I wonder how much of a slowdown they would bring overall even with a well designed plugin... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29392 --- Comment #8 from Fridolin Somers <fridolin.somers@biblibre.com> --- Bug 29672 pushed master for 22.11. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Patch doesn't apply --- Comment #9 from David Nind <david@davidnind.com> --- The patch no longer applies (not unexpected!). Not sure about the comments after the patch 8-) git bz apply 28173 Bug 28173 - Add plugin hooks object_store_pre and object_store_post 119872 - Bug 28173: Add plugin hooks object_store_pre and object_store_post Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28173: Add plugin hooks object_store_pre and object_store_post Using index info to reconstruct a base tree... M Koha/Object.pm A t/lib/Koha/Plugin/Test.pm Falling back to patching base and 3-way merge... Auto-merging t/lib/plugins/Koha/Plugin/Test.pm CONFLICT (content): Merge conflict in t/lib/plugins/Koha/Plugin/Test.pm Auto-merging Koha/Object.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 28173: Add plugin hooks object_store_pre and object_store_post ... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- While I've had my reservations about this, having some kind of "pre" and "post" store hooks would be amazing... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119872|0 |1 is obsolete| | --- Comment #11 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 163509 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163509&action=edit Bug 28173: Add plugin hooks object_store_pre and object_store_post These two hooks are called in Koha::Object::store, which means any object stored in the database will trigger these hooks. object_store_pre is called just before `update_or_insert`, allowing to modify the object before sending the INSERT/UPDATE query object_store_post is called just after `update_or_insert`, so it can be used for any actions that require to have the final object, like indexing, logging, ... Test plan: 1. prove t/db_dependent/Koha/Plugins/Object_hooks.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- Patch rebased on master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #13 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 163516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163516&action=edit Bug 28173: [DO NOT PUSH] Benchmark script -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- I ran the attached benchmark script in 3 different scenario: 1) master (c6fa96eeca) 2) with the patch, with 2 plugins, 1 enabled but without the object_store_* hooks, 1 with both hooks but disabled. 3) same as 2, but with both plugins enabled The code inside object_store_* hooks was very simple (1 call to $object->_type and 1 call to $object->make_column_dirty) 1) The time per `store` call is between 91µs and 104µs 2) The time per `store` call is between 98µs and 110µs 3) The time per `store` call is between 104µs and 115µs There is a noticeable slowdown, which is expected. I think 10µs per call is negligible if we only have a few store calls per http request, but I don't know how many store calls we have on a typical request. Does anyone know ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34943 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #15 from Fridolin Somers <fridolin.somers@biblibre.com> --- I linked to Bug 34943 recently pushed master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@trust-box.at -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28655 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olivier.hubert@inlibro.com --- Comment #16 from Olivier Hubert <olivier.hubert@inlibro.com> --- Re: comment #14 In my (limited) experience, there is no such thing as a "typical" Koha request. Some requests might trigger several (10-20+ store calls), whereas others will only trigger a few. Even with the performance improvement, I'm not sure this could work in all environments. For example, we have installations with 7-8+ plugins. Would system-wide option to enable / disable the hooks make sense? If this is accepted as it stands, I would also prefer the hooks to be named "before_object_store" and "after_object_store", in order to be more consistent. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to Olivier Hubert from comment #16)
In my (limited) experience, there is no such thing as a "typical" Koha request. Some requests might trigger several (10-20+ store calls), whereas others will only trigger a few. Even with the performance improvement, I'm not sure this could work in all environments. For example, we have installations with 7-8+ plugins.
I have this same fear.
Would system-wide option to enable / disable the hooks make sense?
As I was reading your comment, I was thinking this same thing! I think this is a really interesting idea actually.
If this is accepted as it stands, I would also prefer the hooks to be named "before_object_store" and "after_object_store", in order to be more consistent.
Makes sense to me too. (Although the wiki is down right now, so I'm not able to do a good review of the existent hooks. A bit telling that we need to rely on the wiki to know what hooks there are too. If we did have a system-wide configuration of hook availability, it would be a nice way of easily seeing what hooks a system implements...) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org