Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.3] Added database slave failover #15553

Merged
merged 5 commits into from
Nov 2, 2016
Merged

[5.3] Added database slave failover #15553

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a839cf
Added database slave failover to laravel framework
Data33 Sep 21, 2016
9f8dfee
Replaced tabs with spaces
Data33 Sep 22, 2016
ae4b2f9
Fixed a couple of code style issues
Data33 Sep 22, 2016
bd9acdf
Added empty comment and sorted imports by length
Data33 Sep 22, 2016
c9125f3
PDOException is now handled by ExceptionHandler
Data33 Sep 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 53 additions & 6 deletions src/Illuminate/Database/Connectors/ConnectionFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Illuminate\Database\Connectors;

use PDO;
use PDOException;
use Illuminate\Support\Arr;
use InvalidArgumentException;
use Illuminate\Database\MySqlConnection;
Expand Down Expand Up @@ -95,6 +96,58 @@ protected function createReadPdo(array $config)
* @return \Closure
*/
protected function createPdoResolver(array $config)
{
if (array_key_exists('host', $config)) {
return $this->createPdoResolverWithHosts($config);
}

return $this->createPdoResolverWithoutHosts($config);
Copy link
Member

@taylorotwell taylorotwell Oct 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why would a database configuration ever not have a host property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could see, sqlite configurations don't have a host property, hence the two different methods.

}

/**
* Create a new Closure that resolves to a PDO instance with a specific host or an array of hosts.
*
* @param array $config
* @return \Closure
*/
protected function createPdoResolverWithHosts(array $config)
{
return function () use ($config) {
if (! is_array($config['host'])) {
$hosts = [$config['host']];
} else {
$hosts = $config['host'];
shuffle($hosts);
}

$lastHost = end($hosts);
foreach ($hosts as $host) {
$config['host'] = $host;

try {
return $this->createConnector($config)->connect($config);
} catch (PDOException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an excellent point.
I'll edit the PR first thing tomorrow morning to fix that

if ($host !== $lastHost) {
$this->container->make('Illuminate\Contracts\Debug\ExceptionHandler')->report($e);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

An empty comment should be added.

} catch(PDOException $e) {
    //
}

}

if (empty($hosts)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not making this check before the foreach loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

throw new InvalidArgumentException('Database hosts array cannot be empty');
}

throw $e;
Copy link
Member

Choose a reason for hiding this comment

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

What are you throwing here? You're not even in a try catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

};
}

/**
* Create a new Closure that resolves to a PDO instance where there is no configured host.
*
* @param array $config
* @return \Closure
*/
protected function createPdoResolverWithoutHosts(array $config)
{
return function () use ($config) {
return $this->createConnector($config)->connect($config);
Expand All @@ -111,12 +164,6 @@ protected function getReadConfig(array $config)
{
$readConfig = $this->getReadWriteConfig($config, 'read');

if (isset($readConfig['host']) && is_array($readConfig['host'])) {
$readConfig['host'] = count($readConfig['host']) > 1
? $readConfig['host'][array_rand($readConfig['host'])]
: $readConfig['host'][0];
}

return $this->mergeReadWriteConfig($config, $readConfig);
}

Expand Down