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

Already on GitHub? Sign in to your account

Adding experimental retry functionality. #35

Merged
merged 1 commit into from Dec 23, 2011

Conversation

Projects
None yet
4 participants
Owner

jwage commented Dec 17, 2011

In our application at OpenSky we're dealing with errors from mongodb connections and cursors when our replica set changes. For example when a master steps down, it may take a few seconds for a slave to be elected as primary so during that time we need to handle this and retry functions that can throw MongoConnectionException and MongoCursorException.

Sometimes randomly we've gotten the errors as well so I think we just need to handle it and retry when it happens. For example when iterating over a cursor, you may get an exception from one getNext() call but simply trying again right afterwards it will work.

@jwage jwage commented on the diff Dec 17, 2011

lib/Doctrine/MongoDB/Cursor.php
@@ -247,4 +272,23 @@ public function getSingleResult()
$this->reset();
return $result ? $result : null;
}
-}
+
+ protected function retry(\Closure $retry)
+ {
+ if ($this->numRetries !== null && $this->numRetries !== false) {
+ for ($i = 1; $i <= $this->numRetries; $i++) {
+ try {
+ return $retry();
+ break;
+ } catch (\MongoException $e) {
@jwage

jwage Dec 17, 2011

Owner

Need to make this catch more specific.

@jwage jwage commented on the diff Dec 17, 2011

lib/Doctrine/MongoDB/Collection.php
@@ -568,4 +629,23 @@ public function __toString()
{
return $this->mongoCollection->__toString();
}
-}
+
+ protected function retry(\Closure $retry)
+ {
+ if ($this->numRetries !== null && $this->numRetries !== false) {
+ for ($i = 1; $i <= $this->numRetries; $i++) {
+ try {
+ return $retry();
+ break;
+ } catch (\MongoException $e) {
@jwage

jwage Dec 17, 2011

Owner

Need to make this catch more specific.

Member

kriswallsmith commented Dec 17, 2011

What do you think about moving the retry logic to a collaborator? That way users could opt-in to the whole thing. It could also give us a much cleaner way to accomplish logging by getting rid of all the Loggable* subclasses and creating Loggable* collaborators (see #34).

Owner

jwage commented Dec 17, 2011

Well, it's slightly different for each retry, it catches a different exception in different cases so I am not sure if we can have a shared retry method. As for the logging stuff, thats the way logging used to be but it was changed by someone to use the sub classes.

Member

kriswallsmith commented Dec 17, 2011

Would you be able to do what you need to do here with a protected method like...

protected function callDelegate($method, $arguments);

You could have retry logic in there and I could also use that method to accomplish the enhanced logging in #34 in the Loggable* subclasses. I'm not able to use your retry method for logging because there's no visibility into what the closure is doing.

Owner

jwage commented Dec 17, 2011

I am not sure. I'd have to see a proposed patch.

Member

kriswallsmith commented Dec 18, 2011

I'll put something together.

@kriswallsmith kriswallsmith commented on the diff Dec 19, 2011

lib/Doctrine/MongoDB/Collection.php
@@ -568,4 +629,23 @@ public function __toString()
{
return $this->mongoCollection->__toString();
}
-}
+
+ protected function retry(\Closure $retry)
+ {
+ if ($this->numRetries !== null && $this->numRetries !== false) {
+ for ($i = 1; $i <= $this->numRetries; $i++) {
@kriswallsmith

kriswallsmith Dec 19, 2011

Member

How does this behave if numRetries is true?

Member

kriswallsmith commented Dec 19, 2011

I've merged this work into #34. Please let me know what you think.

@jwage jwage merged commit 5ccb182 into master Dec 23, 2011

Contributor

meonkeys commented Jun 20, 2012

Why is this necessary? http://php.net/class.mongocursorexception says "The driver will automatically retry...", and doesn't doctrine-mongodb use the Mongo PHP extension?

Owner

jmikola commented Nov 6, 2012

@meonkeys: the PHP driver will internally watch its connections and re-connect as needed, but it's still possible for exceptions to be raised at runtime. This PR simply catches those exceptions and allows the query to be re-executed after the driver has a chance to recover. It should alleviate the need to manually try/catch queries in your application code, which is what most folks do to properly handle replica set failovers.

Contributor

meonkeys commented Nov 6, 2012

It should alleviate the need to manually try/catch queries in your application code, which is what most folks do to properly handle replica set failovers.

*gulp*

I had no idea I was supposed to be doing that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment