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

Memberlist: Avoid checking for conflicting tokens if no tokens changed. #91

Merged
merged 2 commits into from
Dec 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* [ENHANCEMENT] Replace go-kit/kit/log with go-kit/log. #52
* [ENHANCEMENT] Add spanlogger package. #42
* [ENHANCEMENT] Add runutil.CloseWithLogOnErr function. #58
* [ENHANCEMENT] Optimise memberlist receive path when used as a backing store for rings with a large number of members. #76 #77 #84
* [ENHANCEMENT] Optimise memberlist receive path when used as a backing store for rings with a large number of members. #76 #77 #84 #91
* [ENHANCEMENT] Memberlist: prepare the data to send on the write before starting counting the elapsed time for `-memberlist.packet-write-timeout`, in order to reduce chances we hit the timeout when sending a packet to other node. #89
* [ENHANCEMENT] flagext: for cases such as `DeprecatedFlag()` that need a logger, add RegisterFlagsWithLogger. #80
* [BUGFIX] spanlogger: Support multiple tenant IDs. #59
Expand Down
19 changes: 18 additions & 1 deletion ring/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,15 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
otherIngesterMap := other.Ingesters

var updated []string
tokensChanged := false

for name, oing := range otherIngesterMap {
ting := thisIngesterMap[name]
// ting.Timestamp will be 0, if there was no such ingester in our version
if oing.Timestamp > ting.Timestamp {
if !tokensEqual(ting.Tokens, oing.Tokens) {
tokensChanged = true
}
oing.Tokens = append([]uint32(nil), oing.Tokens...) // make a copy of tokens
thisIngesterMap[name] = oing
Comment on lines 214 to 215
Copy link
Contributor

Choose a reason for hiding this comment

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

if the tokens have not changed, is this necessary?

Copy link
Member

Choose a reason for hiding this comment

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

This updates timestamp and state too.

updated = append(updated, name)
Expand Down Expand Up @@ -241,7 +245,7 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
}

// resolveConflicts allocates lot of memory, so if we can avoid it, do that.
if conflictingTokensExist(thisIngesterMap) {
if tokensChanged && conflictingTokensExist(thisIngesterMap) {
resolveConflicts(thisIngesterMap)
}

Expand Down Expand Up @@ -303,6 +307,19 @@ func normalizeIngestersMap(inputRing *Desc) {
}
}

// tokensEqual checks for equality of two slices. Assumes the slices are sorted.
func tokensEqual(lhs, rhs []uint32) bool {
if len(lhs) != len(rhs) {
return false
}
for i := 0; i < len(lhs); i++ {
if lhs[i] != rhs[i] {
return false
}
}
return true
}

var tokenMapPool = sync.Pool{New: func() interface{} { return make(map[uint32]struct{}) }}

func conflictingTokensExist(normalizedIngesters map[string]InstanceDesc) bool {
Expand Down