Skip to content
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

Decouple routing logic from address set #200

Merged
merged 4 commits into from
Jul 18, 2017
Merged

Decouple routing logic from address set #200

merged 4 commits into from
Jul 18, 2017

Conversation

lutovich
Copy link
Contributor

No description provided.

This commit introduces a separate load balancing strategy abstraction
responsible for address selection.
Commit changes `TryAcquire` with out parameter to simple `Acquire` that
either returns a connection or null.
@lutovich lutovich requested a review from ali-ince July 17, 2017 17:43
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

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

Your changes looks ok. A few comments only.

value = _items[_index++];
}
return true;
_items = new List<T>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also make this assignment a critical section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. It is fine for #Clear() to assign because it is an atomic action, but it is not fine for it to not coordinate with other modification methods like #Add() or #Remove().

/// Not thread safe
/// </summary>
/// <returns></returns>
string ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for a ToString() declaration as it is part of Object class that can be overrided.

Re-assignment of the internal list needs to be done while holding a lock
so that it is coordinated and isolated from other modifications like
`#Add()` or `#Remove()`.
@lutovich
Copy link
Contributor Author

@ali-ince comments should now be addresses. Thank you.

@ali-ince ali-ince merged commit a7a9087 into 1.5 Jul 18, 2017
@lutovich lutovich deleted the 1.5-lb-infra branch July 18, 2017 08:54
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.

None yet

2 participants