MasterSlaveConnection::ping() throws PDOException #3313
Conversation
|
|
||
| return true; | ||
| } catch (DBALException $e) { | ||
| } catch (PDOException | DBALException $e) { |
There was a problem hiding this comment.
When does a PDOException occur? I believe we upcast those, so this is fixing a symptom
There was a problem hiding this comment.
we may stay with DBALException only for case when we will catch Throwable and convert them to the DBALException in the query methods.
| */ | ||
| public function ping() | ||
| { | ||
| $this->connect('master'); |
There was a problem hiding this comment.
Seems incorrect to me: it should simply ping whichever connection is already selected, as otherwise we may attempt to select the slave and then not get a valid response
| $statement = $this->_conn->query(...$args); | ||
| try { | ||
| $statement = $this->_conn->query(...$args); | ||
| } catch (Throwable $e) { |
There was a problem hiding this comment.
Same as above: we upcast these, so if we need to further wrap it there's something else going on
There was a problem hiding this comment.
I follow the same idea from the regular Connection class https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L1011-L1015
|
is the latest build random? oO |
|
@sidz no, it just errored. I restarted the needed jobs. |
|
@morozov doesn't help :( |
|
@sidz should be related to https://www.traviscistatus.com/incidents/k35bqcmm5274. |
|
@sidz please try rebasing on the latest master. |
|
@morozov done |
|
@sidz the Travis CI build is green but AppVeyor is all red. I've restarted it to see if it's something accidental. |
|
@Ocramius ping for re-review |
|
Irrelevant as of #4128. |
3 participants
Summary
Fix existing issue with uncaught exception in the
MasterSlaveConnection::query.Also looks like
pingmethod in the MasterSlaveConnection class will send pings to the slave connection:https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php#L124-L133
I've marked it as BC Break as
pingmethod will change it behavior in theMasterSlaveConnectionand will ping master instead of slavep.s. this PR covers 2 issues: #3118 and #2769