Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make retry strategies cluster aware #329
Make retry strategies cluster aware #329
Changes from 20 commits
d704b07
c9c578c
73d812c
72c3b00
946e1f0
cf0b482
fc5eb12
e6370ae
0d00473
17d7818
291e7eb
0c4ca24
12ab189
e3b3cd8
9c5348a
05d8947
3156e02
db11031
784960e
2d8d093
93801a8
5eda5ad
b23202e
d3b629a
c99628f
a515a74
d54ab5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we add
:random
here? Can't a retry strategy implement it by itself?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only for internal use, not exposed to the outside. It's basically a fallback for when a
:target_connection
doesn't exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether I understood "implement it by itself"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, a retry strategy can implement choosing a random node if
:target_connection
doesn't exist, right? Unless we have a strong reason, let's just remove this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, we pop the
:target_connection
before callingretry_strategy.retry
and then doafter calling
retry_strategy.retry
, so that if it is empty, we'll be assigning it a random node. This is to have a mechanism to initially select the first node from load balancing strategy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whatyouhide But using
nil
instead of:random
also works, as in d54ab5b, if that's what you meant