Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[WIP] Added deadlock check #388

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

alex88 commented Oct 19, 2013

I've added the interface method and drivers implementation to check if
a given exception is a deadlock so can be retried by the pull request
on ORM doctrine/doctrine2#806

At the moment I've only implemented MySql checks.

I'm not sure about where to place the tests, the only I've found about Drivers are those regarding OCI8. Where should I put those?

Added deadlock check interface method and driver implementation
I've added the interface method and drivers implementation to check if
a given exception is a deadlock so can be retried by the pull request
on ORM doctrine/doctrine2#806

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-635

We use Jira to track the state of pull requests and the versions they got
included in.

@deeky666 deeky666 commented on the diff Oct 19, 2013

lib/Doctrine/DBAL/Driver/Mysqli/Driver.php
@@ -67,4 +67,16 @@ public function getDatabase(\Doctrine\DBAL\Connection $conn)
return $params['dbname'];
}
+
+ /**
+ * {@inheritdoc}
+ */
+ public function isDeadlockException(\Doctrine\DBAL\DBALException $e)
+ {
+ if (strstr($e->getMessage(), 'Deadlock found when trying to get lock; try restarting transaction') && $e->getCode() == 1213) {
@deeky666

deeky666 Oct 19, 2013

Member

I doubt checking for a message string is the way to go here. This feels unsafe and "hackish" IMO. Also you should import \Doctrine\DBAL\DBALException in a use statement at the beginning (affects the other drivers you modified, too).

Contributor

alex88 commented Oct 19, 2013

OK I'll leave only the code error number check and I'll add the use statement.
About the tests? Any advice on how to proceed?

@deeky666 deeky666 commented on the diff Oct 19, 2013

lib/Doctrine/DBAL/Driver.php
@@ -72,4 +72,14 @@ public function getName();
* @return string The name of the database.
*/
public function getDatabase(Connection $conn);
+
+ /**
+ * Checks if the exception provided is a deadlock
+ * and can be safely retryied
+ *
+ * @param \Doctrine\DBAL\DBALException $e
+ *
+ * @return boolean
+ */
+ public function isDeadlockException(DBALException $e);
@deeky666

deeky666 Oct 19, 2013

Member

This is the second modification to the driver interface for handling driver exceptions (considering PR #364 as the first one) and I'd like to start the discussion once again about moving such things to a DriverException interface instead of bloating the driver interface for such purposes (see the discussion in PR #364). ;)

Contributor

alex88 commented Oct 19, 2013

Yeah that should be better, calling something like $e->isDeadlock() would be way cleaner on orm side instead of fetching the driver and checking the exception against it.

Member

deeky666 commented Oct 19, 2013

I think you should create test classes for each driver (just like for the Oracle driver) and test your methods. Though, I'm not really sure putting the method into the driver interface is the best approach here (see my comment above). But I'll leave that discussion open to the other repo collabs, as it was already denied for PR #364 ;) Maybe this second modification is a reason for rethinking about that.

Contributor

alex88 commented Oct 19, 2013

Sure, I'll wait some more feedbacks to see how to go further with this.

Member

stof commented Oct 24, 2013

I don't think it can be done in the exception itself as $e->isDeadlock():

  • putting logic in the Exception instance looks wrong to me
  • it would require creating different exception classes for each driver (and making sure the right one is used each time).

The discussion in the other PR was talking about creating a new service class responsible of examinating exceptions instead of putting the logic directly in the driver, not of putting this logic in the exception.

Contributor

alex88 commented Oct 24, 2013

Are you talking about my PR or deeky666's one? So I should create a new class that given the driver and the exception checks if it's a deadlock one?

We should instantiate it during the connection setup? Maybe something like $connection->getExceptionCategorizer()?

Owner

beberlei commented Oct 26, 2013

This should be merged into GH-364.

@beberlei beberlei closed this Oct 26, 2013

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