Reuse existing MySQL connection

[fondrenlibrary]

Update DBService.php of all modules except the Report one to reuse existing MySQL database connection. A new private static property called $currDBH is introduced into the DBService class. This property is used to store existing database connection. In order to keep number of changes small, the original instance property $db is kept and its value is set with the value of $currDBH whenever existing connection is available. Without this fix, Coral opens as many database connections as number of child records until request for new connection is silently rejected and sees no records returned at all when processing child records (e.g., License record).

[t4k]

I think this is a great opportunity to start implementing improvements pointed out by @tuxayo in #77:

Duplication between modules

Whether it’s in the database or in the code itself, having now everything in the same repo allows to share stuff.

For example it seems that generic classes like DBService or other abstractions are 99% the same between modules. So for example when finishing the conversion from mysql to mysqli in Management, it would have been useful to study the possibility of merging the DB related classes with another module that was already converted. Instead of doing the same work and contributing to make them diverge in meaningless ways.

Could we get the common DBService.php file updated along with the others to start bringing it in line so that it could eventually be the single DBService.php file across CORAL?

Do others think this is the right approach to begin moving in this direction?

[jeffnm]

Yes! I think DBService.php would be a great place to start using a true common architecture.

[veggiematts]

I think these two should be addressed separately:

• Reuse database connection
• Have a common architecture

The first one could be quickly fixed (and is needed imho).
The latter would be nice but needs more work.

About reusing the mysql connection, I already made a patch (should have done a pull request, my bad), for information, it's there: biblibre@18d0172 (from #145 )

[fondrenlibrary]

@veggiematts , I agree to your two step approach. As to the patch you made much earlier, it's very similar to this one, except that your patch contains changes in other files not only in DBService.php. Interesting~

[t4k]

Just to clarify my comment above…

I’m not suggesting we switch anything over to a common architecture yet. That is too big of a step that will require lots of review and testing.

I’m just asking that going forward, we makes steps in that direction. If we always put it off, it will never happen. Starting small should make it more manageable.

In this case it would only require adding the changes made in the individual DBService.php files into the common one.

That common DBService.php file wouldn’t be called by any of the modules at this time, but it starts a pattern of working toward the goal of a common architecture.

[fondrenlibrary]

Totally agreed. Mang

…

On 6/23/2017 12:19 PM, Tommy Keswick wrote: Just to clarify my comment above… I’m not suggesting we switch anything over to a common architecture yet. That is too big of a step that will require lots of review and testing. I’m just asking that going forward, we makes steps in that direction. If we always put it off, it will never happen. Starting small should make it more manageable. In this case it would only require adding the changes made in the individual DBService.php files into the common one. That common DBService.php file wouldn’t be called by any of the modules at this time, but it starts a pattern of working toward the goal of a common architecture. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#245 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK7jvK0CxBJhhEgfEPFDrztpkKK_x3Qvks5sG_OlgaJpZM4OBL6S>.

[PaulPoulain]

t4k: can I say that the short term vision is fixing the performance delay, while the long term one is to switching to a better architecture, small step by small step ?
I'm totally in favor of the small steps way of doing things.

[fondrenlibrary]

, as you can see in this screen snapshot, Invoice 2017 has 337 documents as its sub records. With Current Coral , when you click "Show All Documents For This Parent", the 337 child records won't show up as expected, instead the page will only show zero sub records, because by default, MYSQL only supports 128 concurrent connections while current Coral will open 337 connections, though you tweak MySQL parameters to support larger number of active connections. To reproduce this error, you can create a license record with the Licensing module and then create/upload a document record followed by creating a large number (>300) of sub documents using the one created earlier as their parent and see the issue. After this PR being applied, click the link again you should see all the sub document records. For other modules and again Licensing , simply test that the PR doesn't t break any existing functions. Feel free to make amendment to this simple PR and make the fixing code more decent.

[t4k]

Can anyone confirm this was resolved with #292? If so, please comment and close the PR.

[fondrenlibrary]

According to the files changed by PR #292 , it only solves the problem with the resource module. If the owner of #292 could duplicate the change to DBSevices.php to other modules including licensing, then this PR can be closed after test.

[t4k]

Closing this as #343 was merged.