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

support adding/removing clients from LBClient #1243

Merged
merged 8 commits into from Apr 1, 2022

Conversation

treethought
Copy link
Contributor

@treethought treethought commented Mar 10, 2022

addresses #504 and is a followup to #556, addressing the outstanding feedback.

Adds support for adding/removing clients from LBClient utilizing a mutex as well as a user-defined callback for determining which HostClients to be removed

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

You also have add Lock/Unlock to .get. And since it will be more than 99% reads using the Mutex I would a sync.RWMutex

lbclient.go Show resolved Hide resolved
lbclient.go Outdated
cc.mu.Lock()
for idx, cs := range cc.cs {
if rc(cs.c) {
cc.cs = append(cc.cs[:idx], cc.cs[idx+1])
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 26, 2022

Choose a reason for hiding this comment

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

This is not a correct way to filter a slice. See: https://go.dev/play/p/9y9DyCJ_xDD

Copy link
Contributor Author

@treethought treethought Mar 26, 2022

Choose a reason for hiding this comment

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

ah, my bad. addressed

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

That is still not a correct way to filter a slice, see: https://go.dev/play/p/JM8EdsOgVMK
For a correct way see: https://github.com/golang/go/wiki/SliceTricks#filter-in-place
But make sure you set the unused entries at the end to nil before you resize the slice so there are no references to those clients anymore.

@treethought
Copy link
Contributor Author

treethought commented Mar 31, 2022

thanks for the tip, updated the filtering. https://go.dev/play/p/ZwhI3M2SDI9

lbclient.go Outdated Show resolved Hide resolved
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
@erikdubbelboer erikdubbelboer merged commit e4a541f into valyala:master Apr 1, 2022
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants