[Bug 14375] New: DBIx::Connector should be stored in C4::Context instead of dbh
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Bug ID: 14375 Summary: DBIx::Connector should be stored in C4::Context instead of dbh Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: fridolyn.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |fridolyn.somers@biblibre.co | |m Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #1 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 40063 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40063&action=edit Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |14374 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40063|0 |1 is obsolete| | --- Comment #2 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 40065 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40065&action=edit Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #3 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Are they even used ? I found nothing in sources. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Indranil Das Gupta <indradg@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |indradg@gmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40065|0 |1 is obsolete| | --- Comment #4 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 40244 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40244&action=edit Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Note that mysql_enable_utf8 option is passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #5 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- I've found a mandatory change : mysql_enable_utf8 option must be passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |critical -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13699 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #6 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Fridolin SOMERS from comment #0)
We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection.
Does it mean you are running this patch in production yet? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart@biblibre.com> --- There is a lot of occurrences of _new_dbh in tests: git grep _new_dbh | wc -l 17 I'd say keep the original name "_new_dbh", why do you want to change it? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #8 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (In reply to Jonathan Druart from comment #6)
(In reply to Fridolin SOMERS from comment #0)
We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection.
Does it mean you are running this patch in production yet?
Not in production but in test for it -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14600 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40244|0 |1 is obsolete| | --- Comment #9 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 41267 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41267&action=edit Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh DBIx::Connector doc about $conn->dbh() indicates he will manage database handler cache and reconnection : http://search.cpan.org/perldoc?DBIx%3A%3AConnector#dbh So C4::Context should not store dbh object but DBIx::Connector object and get handler from it. We experimented issues with SIP server, since its database connections will run all day long. Looks like DBIx::Connector manages very well the reconnection. Note that mysql_enable_utf8 option is passed in DBIx::Connector creation so it can be set again when the connector recreates a dbh. This patch has to drop methods set_dbh() and restore_dbh() because they seem to be impossible to implement with this changes. Test plan : - Check database connection does not fail for pages - Check database connection does not fail for SIP Server - One may check its ok with Plack -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #10 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- My patch suppressed a few obsolete lines in C4:Context::preference. Those line have nothing to do directly with the problem of this bug so I restore them -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olli-antti.kivilahti@jns.fi --- Comment #11 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Hi there! I like your approach, you are doing the right thing. DBIx::Connector is said to be more robust and I am happy to hear it has solved your issues. However there is an issue with this patch that needs addressing if you want a sign off from me. First of all the naming convention is changed which can cause a lot of issues for everybody. Some tests, custom modifications etc. can depend on $context->{dbh}. The new name doesn't help increase clarity IMHO and we should stick to the old "dbh". - return $context->{"dbh"}; + return $context->{'DBconn'}->dbh; Same issue here -sub _new_dbh +sub _new_DBconn _new_dbh is addressed directly from many places (atleast I do) regardless of it being a private method. This is to circumvent bugs/limitations of the C4::Context->dbh()-system. No reason to change it. Because of these things a small and beautiful, yet invaluably important fix is drowned into a large commit changing too many things for reasons I do not agree of. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #12 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 41437 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41437&action=edit Bug 14375 - Use mock on dbh instead of _new_dbh This patch changes in test modules the mock on database connexion. Instead of mocking method _new_dbh (removed by main patch), mock directly method dbh. Always use t::lib::Mocks::mock_dbh() for that. To test, just run prove on modified test modules. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #13 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- I've corrected test modules that mock database connexion. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #14 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 41439 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41439&action=edit test module Test module. It saves a record and tries to get it each 15 minutes 4 times. Without patch, it fails with : database has run away. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Fridolin SOMERS from comment #14)
Created attachment 41439 [details] test module
Test module.
It saves a record and tries to get it each 15 minutes 4 times. Without patch, it fails with : database has run away.
Why 900sec? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #16 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (In reply to Jonathan Druart from comment #15)
(In reply to Fridolin SOMERS from comment #14)
Created attachment 41439 [details] test module
Test module.
It saves a record and tries to get it each 15 minutes 4 times. Without patch, it fails with : database has run away.
Why 900sec?
It is to loose the DB connexion. Maybe a few minutes is enought. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14778 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Fridolin, Could you have a look at the patch I have just submitted on bug 14778? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |chris@bigballofwax.co.nz --- Comment #18 from Chris Cormack <chris@bigballofwax.co.nz> --- Please see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14778 The Author of DBIx::Class recommends we shouldn't use DBIx::Connector and I tend to agree -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 --- Comment #19 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (In reply to Chris Cormack from comment #18)
Please see
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14778
The Author of DBIx::Class recommends we shouldn't use DBIx::Connector and I tend to agree
Oki, this is even better than this patch, great. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|In Discussion |RESOLVED Resolution|--- |DUPLICATE --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- *** This bug has been marked as a duplicate of bug 14778 *** -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375 Bug 14375 depends on bug 14374, which changed state. Bug 14374 Summary: dont use mysql_auto_reconnect with DBIx::Connector http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14374 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |RESOLVED Resolution|--- |INVALID -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org