Ring: Add HasReplicationSetChangedWithoutStateOrAddr
#464
Conversation
pr00se
changed the title
ring.HasReplicationSetChangedWithoutStateOrAddrHasReplicationSetChangedWithoutStateOrAddr
| @@ -21,6 +21,13 @@ func (ts ByAddr) Len() int { return len(ts) } | |||
| func (ts ByAddr) Swap(i, j int) { ts[i], ts[j] = ts[j], ts[i] } | |||
| func (ts ByAddr) Less(i, j int) bool { return ts[i].Addr < ts[j].Addr } | |||
|
|
|||
| // ByID is a sortable list of InstanceDesc. | |||
There was a problem hiding this comment.
I've left ByAddr because it is used outside of the library
| @@ -468,6 +468,17 @@ func HasReplicationSetChangedWithoutState(before, after ReplicationSet) bool { | |||
| }) | |||
| } | |||
|
|
|||
| // Has HasReplicationSetChangedWithoutStateOrAddr returns false if two replications sets | |||
There was a problem hiding this comment.
You don't have to do it as part of this PR but we seem to be accumulating quite a few different methods that all do roughly the same thing. Maybe we could apply the option pattern here.
e.g.
func HasReplicationSetChanged(before, after ReplicationSet, opt... RingOption) bool { /* ... */ }Called like
if ring.HasReplicationSetChanged(a, b, ring.ExcludeAddress(), ring.ExcludeState()) {
/* ... */
}There was a problem hiding this comment.
I had the same thought, but it would mean breaking changes not only for Mimir. Since dskit doesn't have a formal versioning process we'd need to coordinate changing over (or have two implementations for some time). We also don't have an easy way to signal that breaking change to any external users of dskit (are there any?).
We could add something like
func HasReplicationSetChangedFn(opt... ExcludeOption) func (before, after ReplicationSet) bool { /* ... */ }
and replace the usage of HasReplicationSetChangedWithoutXXXXX, but I don't foresee us needing another flavor of this kind of function in the future (there aren't many other fields left to ignore). If we do end up adding a third implementation, though, we should revisit this.
What this PR does:
The existing
ring.HasReplicationSetChangedWithoutStatefunction will detect a replication state change if a member was rebooted and rejoins the ring with a different IP address. In Mimir we want to ignore these changes as, in some cases, we only care about the distribution of the tokens in the ring changing.This PR:
ring.hasReplicationSetChangedExcludingfromByAddrtoByID, since we want to ignore theAddrfield in some situations.ring.HasReplicationSetChangedWithoutStateOrAddrto detect replication set changes ignoring the State and Addr fields.Which issue(s) this PR fixes:
Fixes N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]