[5.3] Added database slave failover #15553
Conversation
Handled edge case where empty array of hosts would break createPdoResolverWithHosts
| } | ||
| } | ||
|
|
||
| if (empty($hosts)) { |
There was a problem hiding this comment.
Why not making this check before the foreach loop.
There was a problem hiding this comment.
If I move the check above the foreach loop it will be tested even if a connection was actually made, which seems redundant to me
It really is mostly a check so $e actually exists, since it's created inside the foreach loop
| try{ | ||
| return $this->createConnector($config)->connect($config); | ||
| } | ||
| catch(\PDOException $e){ |
There was a problem hiding this comment.
\PDOException should be imported.
There was a problem hiding this comment.
I'll fix this in the PR.
| if (array_key_exists('host', $config)) { | ||
| return $this->createPdoResolverWithHosts($config); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
I'll fix this in the PR.
| return function () use ($config) { | ||
| return $this->createConnector($config)->connect($config); | ||
| return $this->createConnector($config)->connect($config); |
There was a problem hiding this comment.
I'll fix this in the PR.
| try { | ||
| return $this->createConnector($config)->connect($config); | ||
| } catch(PDOException $e) { | ||
| } |
There was a problem hiding this comment.
An empty comment should be added.
} catch(PDOException $e) {
//
}| @@ -10,6 +10,7 @@ | |||
| use Illuminate\Database\PostgresConnection; | |||
| use Illuminate\Database\SqlServerConnection; | |||
| use Illuminate\Contracts\Container\Container; | |||
| use PDOException; | |||
There was a problem hiding this comment.
Imports shall be sorted by length.
|
|
||
| try { | ||
| return $this->createConnector($config)->connect($config); | ||
| } catch (PDOException $e) { |
There was a problem hiding this comment.
Would this suppress any other exceptions? Such as the user not having access to the database or the database not existing? If so I'd think we would want to be much more narrow in scope and report all but the actual connection error.
There was a problem hiding this comment.
I guess it could suppress other exceptions.
Letting the exception bubble up would kind of defeat the purpose of having a failover.
Would logging connection errors be enough, or do we need to actually show the user why a specific host could not be connected to?
There was a problem hiding this comment.
Exceptions definitely need to bubble up to at lease the handler so we can report them to things like sentry. Otherwise your primary db could be failing and you would have no idea.
There was a problem hiding this comment.
That's an excellent point.
I'll edit the PR first thing tomorrow morning to fix that
GrahamCampbell
changed the title
|
I've made the requested changes to the pull request. |
| return $this->createPdoResolverWithHosts($config); | ||
| } | ||
|
|
||
| return $this->createPdoResolverWithoutHosts($config); |
There was a problem hiding this comment.
Why would a database configuration ever not have a host property?
There was a problem hiding this comment.
As far as I could see, sqlite configurations don't have a host property, hence the two different methods.
| throw new InvalidArgumentException('Database hosts array cannot be empty'); | ||
| } | ||
|
|
||
| throw $e; |
There was a problem hiding this comment.
What are you throwing here? You're not even in a try catch block.
There was a problem hiding this comment.
The only way for the code to reach that throw statement is if all hosts have failed. If so, the $e variable will be set from the catch block in the last iteration of the foreach loop.
If any host succeeded, the method would have returned already.
|
Why random? Would be good if you could order the list in which you would like the application to try first |
|
Good point. I'll make a new PR and submit later today Edit: #23307 |
|
Our laravel app seems to be failing over from the read replicas over to the Write replica! |
If we have more than one database slave (read hosts) and our application fails to connect to one of them, we want our application to try the next one instead of simply failing.
This commit will try hosts in a random order until one works or until they have all failed.