Skip to content

add round robin fail-over for a client #98

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

Merged

Conversation

ztarvos
Copy link

@ztarvos ztarvos commented Dec 3, 2018

Added socket channel provider implementation that
would fail-over to the next configured server in a
round-robin fashion.

Closes #37

@ztarvos ztarvos requested a review from Totktonada December 3, 2018 12:43
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

I proposed some fixes on the add-round-robin-reconnect-strategy-fixups branch and leave some comments below.

There is unclear thing for me: I got failures with your RetryingClient implementation, while it seems this should not happen. I worked it around, but of course it should be investigated further (at least to understand what is going on).

@ztarvos ztarvos force-pushed the add-round-robin-reconnect-strategy branch from 31a3b89 to 17a941a Compare December 12, 2018 17:34
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

The PR looks good to me.

I merged #99 recently and this PR need to be rebased. I made it on the add-round-robin-reconnect-strategy-rebased branch: resolved trivial conflicts within the rebased commit and added the commit with changes I want you to see (I don't sure whether I did good thing around generics).

Added cluster-ready client implementation that
would fail-over to the next configured server in a
round-robin fashion.

Closes tarantool#37
@ztarvos ztarvos force-pushed the add-round-robin-reconnect-strategy branch from 17a941a to cf16d5c Compare December 13, 2018 15:28
@ztarvos
Copy link
Author

ztarvos commented Dec 13, 2018

I only noticed one missing <V> next to FutureImpl in ExpirableOp<V> declaration. I integrated these changes to my PR.

@Totktonada Totktonada merged commit 935093d into tarantool:master Dec 13, 2018
@ztarvos ztarvos deleted the add-round-robin-reconnect-strategy branch December 14, 2018 08:59
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.

2 participants