Skip to content

Conversation

@fractaloop
Copy link
Contributor

I've added support for maximum retries and updated the tests. Additionally, RedisSpecBase was updated to use the Akka TestKit. The ExponentialReconnectionSettings have been updated and fixed (they would overflow previously).

I'd still like a test for maximum retries but I'm haven't been able to find a solution.

I look forward to your feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we keep it as an Option? As we have generalized maxAttempts, just a lazy val would be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll make those changes.

@fractaloop
Copy link
Contributor Author

I've made those changes, but I'm having a difficult time creating reliable tests since there is no access to the client's address. It would be much easier if RedisConnection captured the Connected() message but I don't want to introduce complexity just so our tests pass. How should I proceed on this?

@fractaloop
Copy link
Contributor Author

Alright, I discovered named clients, which is perfect for our reconnection tests. When implementing this I found a bug in CLIENT SETNAME, which is fixed in this branch as well. This allowed me to reliably kill the connection I wanted and test non-reconnection and reconnections properly.

Is there any more I can do to improve this before merge, or does this fit the bill?

@debasishg
Copy link
Owner

Thanks a lot .. I will try to do a quick browse tonight and let u know ..

@debasishg
Copy link
Owner

Looks good to me .. @guersam - any showstopper for release ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to make it public? I'm a bit afraid of having global mutable state which can be accessed anywhere, even though it's not a part of user-facing interface.

@guersam
Copy link
Collaborator

guersam commented Apr 22, 2014

All the others look good to me, thanks a lot!

@debasishg
Copy link
Owner

cool .. I will merge tomorrow and cut a release ver 0.5 ... Thanks for all the hard work.

@fractaloop
Copy link
Contributor Author

No, that was a mistake from when I was testing earlier. I've made it private again. I'm glad to help.

@debasishg
Copy link
Owner

Looks like I will get some bandwidth only over the weekend. Things are awfully crazy at work :-( .. Is it too much of a delay if I make the release over the weekend ? Apologies, apologies ..

@fractaloop
Copy link
Contributor Author

This weekend would work well, thanks.

debasishg added a commit that referenced this pull request Apr 26, 2014
@debasishg debasishg merged commit 357f869 into debasishg:master Apr 26, 2014
@debasishg
Copy link
Owner

Merged and released 0.5. Thanks to all for the help.

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.

3 participants