Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Reconnect on lost DB connection #307

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Reconnect on lost DB connection #307

wants to merge 15 commits into from

Conversation

@tptrixtop
Copy link
Contributor

@tptrixtop tptrixtop commented Mar 4, 2018 •
edited
Loading

This functionality allow to enable & configure reconnect functionality using options parameter in adapter.

When you are creating adapter for mysqli:

$adapter = new Adapter([
    'driver' => 'Mysqli',
    'database' => '',
    'username' => '',
    'password' => '',
    'options' => [
        'reconnect_tries' => 3
    ]
]);

You can specify reconnect_tries and if during your work connect will be lost adapter will try automatically reconnect to server.

Fix for #302

@alextech
Copy link
Contributor

alextech commented Mar 4, 2018

  1. Needs addition to documentation about new 'reconnect_tries' option.
  2. Needs tests
  3. being a general DB tool, having a feature like this only favoring MySQL ignoring other engines does not make the library look good. At the least, if not have the ability to apply to all supported engines, make a statement in documentation that so far only available in MySQL and maintainer could add "help wanted" tag for other databases.

@tptrixtop
Copy link
Contributor Author

tptrixtop commented Mar 4, 2018 •
edited
Loading

I will try to add support for other engines soon.

I know about unit tests) but when i have written that code i had no idea how to test that functionality)

For now i have some ideas, i will try)

@alextech
Copy link
Contributor

alextech commented Mar 4, 2018

Ok, thats fine :) Does seem like a challenge to test!

For documentation, I actually meant adding entry into docs/book markdown sources.

@tptrixtop
Copy link
Contributor Author

Ah, get it) i will add docs about that option to adapter.md

malukenho
malukenho reviewed Mar 6, 2018
View reviewed changes
src/Adapter/Driver/Mysqli/Mysqli.php

return $this;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

-            if (! $mysqli->ping()) {
-                $mysqli = $this->connection
-                    ->disconnect()
-                    ->connect()
-                    ->getResource();
-
-                if ($mysqli->ping()) {
-                    if ($statement instanceof Statement) {
-                        $statement->initialize($mysqli);
-                    }
-
-                    return $this;
-                }
-            }
+            if ($mysqli->ping()) {
+                if ($statement instanceof Statement) {
+                    $statement->initialize($mysqli);
+                }
+
+                return $this;
+            }
+
+            $mysqli = $this->connection
+                ->disconnect()
+                ->connect()
+                ->getResource();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better.

Thank you sir)

@ezimuel
Copy link
Contributor

ezimuel commented Apr 3, 2018

@tptrixtop we need some unit test for this new feature. Consider that now we have also integration tests available.

@tptrixtop
Copy link
Contributor Author

Sure, I am now working on same functionality for postgresql, it's almost finished and after i will try add same functionality for MS SQL and i will commit unit tests for all of that changes)

@ezimuel
Copy link
Contributor

ezimuel commented Aug 7, 2018

@tptrixtop we still need tests for this feature.

@tptrixtop
Copy link
Contributor Author

@ezimuel I know sir, i am working on it, right now)

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra michalbundyra mentioned this pull request Jan 16, 2020
Closed
@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#55.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@malukenho malukenho malukenho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tptrixtop @alextech @ezimuel @weierophinney @michalbundyra @malukenho