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

[5.3] Added database slave failover #15553

merged 5 commits into from
Nov 2, 2016

Conversation

Data33
Copy link
Contributor

@Data33 Data33 commented Sep 22, 2016

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.

Handled edge case where empty array of hosts would break createPdoResolverWithHosts
}
}

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

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.

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 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.

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports shall be sorted by length.


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

@GrahamCampbell GrahamCampbell changed the title [5.3] Added database slave failover to laravel framework [5.3] Added database slave failover Oct 7, 2016
@Data33
Copy link
Contributor Author

Data33 commented Oct 13, 2016

I've made the requested changes to the pull request.
Do I need to make more changes to this PR or is it ready to be merged?

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

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

@taylorotwell taylorotwell Oct 13, 2016

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.

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.

@taylorotwell taylorotwell merged commit c9125f3 into laravel:5.3 Nov 2, 2016
@simplenotezy
Copy link

Why random? Would be good if you could order the list in which you would like the application to try first

@Data33
Copy link
Contributor Author

Data33 commented Feb 26, 2018

Good point. I'll make a new PR and submit later today

Edit: #23307

@abbood
Copy link

abbood commented Aug 2, 2022

Our laravel app seems to be failing over from the read replicas over to the Write replica!

https://stackoverflow.com/questions/73208840/does-laravel-automatically-failover-database-connections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants