Skip to content

Inconsistent behaviour of Desc.AddIngester #1877

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

Closed
codesome opened this issue Dec 4, 2019 · 2 comments · Fixed by #1809
Closed

Inconsistent behaviour of Desc.AddIngester #1877

codesome opened this issue Dec 4, 2019 · 2 comments · Fixed by #1809

Comments

@codesome
Copy link
Contributor

codesome commented Dec 4, 2019

Before the introduction of normalized tokens, Desc.AddIngester used to append tokens to the existing ones. But currently, it works as append when it's not normalized tokens and replace when it's normalized tokens.

cortex/pkg/ring/model.go

Lines 38 to 63 in 08ddf88

// AddIngester adds the given ingester to the ring.
func (d *Desc) AddIngester(id, addr string, tokens []uint32, state IngesterState, normaliseTokens bool) {
if d.Ingesters == nil {
d.Ingesters = map[string]IngesterDesc{}
}
ingester := IngesterDesc{
Addr: addr,
Timestamp: time.Now().Unix(),
State: state,
}
if normaliseTokens {
ingester.Tokens = tokens
} else {
for _, token := range tokens {
d.Tokens = append(d.Tokens, TokenDesc{
Token: token,
Ingester: id,
})
}
sort.Sort(ByToken(d.Tokens))
}
d.Ingesters[id] = ingester
}

Looking at places where Desc.AddIngester is used, append seems to be the desired behaviour. Discovered in #1750 and @pstibrany plans to fix it in #1809

@pstibrany
Copy link
Contributor

Looking at places where Desc.AddIngester is used, append seems to be the desired behaviour.

append was desired behaviour only when not doing normalization. Since we're removing that option, AddIngester will now always replace the tokens. That requires small change in autoJoin method, but all other places are unaffected. This is how I implemented it in #1809

@codesome
Copy link
Contributor Author

codesome commented Dec 4, 2019

Oh okay. Yes, autoJoin was the only place it could cause a problem. Didn't notice that change in the PR (I didn't have a look tbh :) )

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 a pull request may close this issue.

2 participants