Exponential backoff for recovery retry interval #309
Conversation
|
@vikinghawk Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@vikinghawk Thank you for signing the Contributor License Agreement! |
| for (String consumerTag : consumerTags) { | ||
| this.connection.deleteRecordedConsumer(consumerTag); | ||
| } | ||
| // TODO what about any other recorded queues and bindings that were owned by this channel? They will now cause recovery exceptions. |
There was a problem hiding this comment.
Queues and bindings are not "owned" by channels in any way. Consumers are channel state, however. Queues and bindings are tracked by connections (in part because queues can be owned by connections — when they are exclusive — and in part because declaring something on one channel then deleting it on another should leave no traces of that entity.
There was a problem hiding this comment.
Ya thats a bad description. What that is referencing is that any queues or bindings that were created with this channel still have the recorded entities reference this now closed channel and will throw an exception during recovery. I don't know a good way to address this other than having recorded queues/exchanges/bindings use a special "recovery" channel instead of the original creating channel. I assume the only recorded items that would require to be tied to the original channel that created them would be consumers which are cleaned up.
There was a problem hiding this comment.
removed this change.
| this.connection.deleteRecordedConsumer(consumerTag); | ||
| } | ||
| this.connection.unregisterChannel(this); | ||
| recoveryCleanup(); |
There was a problem hiding this comment.
Is this change really related to delay calculation and back-off?
There was a problem hiding this comment.
no, just noticed the recorded consumers weren't being deleted with the 2nd close method like the 1st. These were some changes i had stashed in my repo from looking at some other stuff and got included here.
There was a problem hiding this comment.
removed for now. could be a separate future PR
|
In general this looks promising. |
|
Updated with javadoc, test, and suggested changes. |
| @@ -541,8 +542,7 @@ private RecoveryAwareAMQConnection recoverConnection() throws InterruptedExcepti | |||
| newConn.abort(); | |||
| return null; | |||
| } catch (Exception e) { | |||
| // TODO: exponential back-off | |||
| Thread.sleep(this.params.getNetworkRecoveryInterval()); | |||
| Thread.sleep(this.params.getRecoveryDelayHandler().getDelay(attempts)); | |||
There was a problem hiding this comment.
thoughts on flipping the order of these 2 lines? Should it be delivering the exception before sleeping?
There was a problem hiding this comment.
Let's keep it the way it is.
There was a problem hiding this comment.
Good catch. I think at some point we will have to do it. The way it works right now is a bit weird and can complicate additional manual recovery steps.
| @@ -541,8 +542,7 @@ private RecoveryAwareAMQConnection recoverConnection() throws InterruptedExcepti | |||
| newConn.abort(); | |||
| return null; | |||
| } catch (Exception e) { | |||
| // TODO: exponential back-off | |||
| Thread.sleep(this.params.getNetworkRecoveryInterval()); | |||
| Thread.sleep(this.params.getRecoveryDelayHandler().getDelay(attempts)); | |||
There was a problem hiding this comment.
Let's keep it the way it is.
Isssue #308
First stab at a pluggable recovery retry interval and basic fibonnaci backoff implementation. Let me know your thoughts and i can make changes and add javadoc if this seems like something you guys like.