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.

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

[5.3] Added database slave failover #15553

Merged
merged 5 commits into from
Nov 2, 2016
Merged
Changes from 2 commits
Commits
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
56 changes: 49 additions & 7 deletions src/Illuminate/Database/Connectors/ConnectionFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,56 @@ protected function createReadPdo(array $config)
*/
protected function createPdoResolver(array $config)
{
if (array_key_exists('host', $config)) {
return $this->createPdoResolverWithHosts($config);
}
else{
Copy link
Contributor

Choose a reason for hiding this comment

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

This else is useless.

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'll fix this in the PR.

return $this->createPdoResolverWithoutHosts($config);
}
}

/**
* 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);
}

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.

\PDOException should be imported.

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'll fix this in the PR.

}
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);
return $this->createConnector($config)->connect($config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

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'll fix this in the PR.

};
}

Expand All @@ -111,12 +159,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