[membership] Replace Ringop with PeerProvider interface #4653
Conversation
| } | ||
|
|
||
| func (r *ringpopHashring) compareMembers(addrs []string) (map[string]struct{}, bool) { | ||
| func (r *ring) compareMembers(addrs []string) (map[string]struct{}, bool) { |
There was a problem hiding this comment.
compareMembersLocked too, as membersMap needs to be protected by the lock
There was a problem hiding this comment.
I'm a bit fuzzy on if duplicates are bad or just unexpected in all this tbh. one place checks, one doesn't, and I'm not sure what two subscribers in either location would imply behaviorally - naively it seems like it'd be fine, though it could be a sign of a double-start or something.
mantas-sidlauskas
force-pushed
the
peer_provider
branch
from
dac754c to
93a1d83
Compare
mantas-sidlauskas
force-pushed
the
peer_provider
branch
from
93a1d83 to
ac9ea4e
Compare
common/membership/hashring.go
Outdated
| r.logger.Error("Failed to send event to subscriber, channel full", tag.Subscriber(name)) | ||
| } | ||
| func (r *ring) refreshWithBackoff() error { | ||
| if r.members.refreshed.After(time.Now().Add(-minRefreshInternal)) { |
There was a problem hiding this comment.
side-benefit of that lock-in-struct pattern: easier to notice incorrect locking!
this needs to be locked or it'll be a racy read. which also brings up a general complication: if you get e.g. 5 calls at once, they can all pass this check and refresh 5x. probably worth fixing?
There was a problem hiding this comment.
removed "with backoff" method, now there's only refreshWithLock()
| select { | ||
| case ch <- change: | ||
| default: | ||
| r.logger.Error("Failed to send listener notification, channel full", tag.Subscriber(name)) |
There was a problem hiding this comment.
is it really an error? or is it expected to be so rare that it may be a sign of problems?
There was a problem hiding this comment.
Locking issues need to be fixed, but the rest are probably "whatever you agree with is fine", it's mostly minor stuff. Looks good overall :)
So I'll approve to unblock whenever you're ready, but feel free to re-request review if you want me to check the changes.
| r.smu.RLock() | ||
| defer r.smu.RUnlock() | ||
|
|
||
| for name, ch := range r.subscribers { |
There was a problem hiding this comment.
@mantas-sidlauskas I was looking at why we never see Ring membership changed log and looks like after this refactoring we stopped notifying the subscribers. Was this intentional or missed?
What changed?
Membership resolver can be started with any
PeerProviderimplementation now.This means:
membership.Resolveris not tightly coupled withRingpopanymorePeerProviderinterface.HostInfo. Labels are moved to part ofringpopversion ofPeerProviderWhy?
This change allows to implement different type of peer provider easily.
How did you test it?
Unit tests, local test
Potential risks
Release notes
Documentation Changes