Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh
Summary: DBIx::Connector should be stored in C4::Context instead of dbh
Status: RESOLVED DUPLICATE of bug 14778
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Keywords:
Depends on: 14374
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-10 13:05 UTC by Fridolin SOMERS
Modified: 2015-11-06 17:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh (6.93 KB, patch)
2015-06-10 13:09 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh (7.05 KB, patch)
2015-06-10 13:12 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh (7.33 KB, patch)
2015-06-17 14:24 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14375 - DBIx::Connector should be stored in C4::Context instead of dbh (7.09 KB, patch)
2015-07-31 14:28 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14375 - Use mock on dbh instead of _new_dbh (13.83 KB, patch)
2015-08-10 09:14 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
test module (930 bytes, application/x-perl)
2015-08-10 12:09 UTC, Fridolin SOMERS
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2015-06-10 13:05:30 UTC
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.
Comment 1 Fridolin SOMERS 2015-06-10 13:09:04 UTC Comment hidden (obsolete)
Comment 2 Fridolin SOMERS 2015-06-10 13:12:50 UTC Comment hidden (obsolete)
Comment 3 Fridolin SOMERS 2015-06-10 13:13:47 UTC
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.
Comment 4 Fridolin SOMERS 2015-06-17 14:24:02 UTC Comment hidden (obsolete)
Comment 5 Fridolin SOMERS 2015-06-17 14:25:07 UTC
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.
Comment 6 Jonathan Druart 2015-06-24 13:49:56 UTC
(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?
Comment 7 Jonathan Druart 2015-06-24 13:55:02 UTC
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?
Comment 8 Fridolin SOMERS 2015-06-24 14:20:03 UTC
(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
Comment 9 Fridolin SOMERS 2015-07-31 14:28:10 UTC
Created attachment 41267 [details] [review]
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
Comment 10 Fridolin SOMERS 2015-07-31 14:29:50 UTC
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
Comment 11 Olli-Antti Kivilahti 2015-08-06 11:26:56 UTC
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.
Comment 12 Fridolin SOMERS 2015-08-10 09:14:51 UTC
Created attachment 41437 [details] [review]
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.
Comment 13 Fridolin SOMERS 2015-08-10 09:15:59 UTC
I've corrected test modules that mock database connexion.
Comment 14 Fridolin SOMERS 2015-08-10 12:09:16 UTC
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.
Comment 15 Jonathan Druart 2015-08-19 13:16:41 UTC
(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?
Comment 16 Fridolin SOMERS 2015-08-20 07:48:05 UTC
(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.
Comment 17 Jonathan Druart 2015-09-03 14:12:36 UTC
Fridolin, Could you have a look at the patch I have just submitted on bug 14778?
Comment 18 Chris Cormack 2015-09-03 21:08:46 UTC
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
Comment 19 Fridolin SOMERS 2015-09-15 09:29:24 UTC
(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.
Comment 20 Tomás Cohen Arazi 2015-11-06 17:24:51 UTC

*** This bug has been marked as a duplicate of bug 14778 ***