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.

Sign up for GitHub

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

Receive: Allow setting hashing algorithm per tenant in hashrings config #5653

Merged
merged 3 commits into from
Feb 7, 2023
Merged

Receive: Allow setting hashing algorithm per tenant in hashrings config #5653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b758905
Fix conflicts
haanhvu Aug 30, 2022
0e87249
Specify overwriting behavior in flag and add validation
haanhvu Sep 25, 2022
7930c75
Add log and doc
haanhvu Feb 5, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#6008](https://github.com/thanos-io/thanos/pull/6008) *: Add counter metric `gate_queries_total` to gate.
- [#5926](https://github.com/thanos-io/thanos/pull/5926) Receiver: Add experimental string interning in writer. Can be enabled with a hidden flag `writer.intern`.
- [#5773](https://github.com/thanos-io/thanos/pull/5773) Store: Support disable cache index header file.
- [#5653](https://github.com/thanos-io/thanos/pull/5653) Receive: Allow setting hashing algorithm per tenant in hashrings config

### Fixed

Expand Down
2 changes: 1 addition & 1 deletion cmd/thanos/receive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ func (rc *receiveConfig) registerFlag(cmd extkingpin.FlagClause) {
cmd.Flag("receive.hashrings", "Alternative to 'receive.hashrings-file' flag (lower priority). Content of file that contains the hashring configuration.").PlaceHolder("<content>").StringVar(&rc.hashringsFileContent)

hashringAlgorithmsHelptext := strings.Join([]string{string(receive.AlgorithmHashmod), string(receive.AlgorithmKetama)}, ", ")
cmd.Flag("receive.hashrings-algorithm", "The algorithm used when distributing series in the hashrings. Must be one of "+hashringAlgorithmsHelptext).
cmd.Flag("receive.hashrings-algorithm", "The algorithm used when distributing series in the hashrings. Must be one of "+hashringAlgorithmsHelptext+". Will be overwritten by the tenant-specific algorithm in the hashring config.").
Default(string(receive.AlgorithmHashmod)).
EnumVar(&rc.hashringsAlgorithm, string(receive.AlgorithmHashmod), string(receive.AlgorithmKetama))

Expand Down
4 changes: 3 additions & 1 deletion docs/components/receive.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,9 @@ Flags:
the hashring configuration.
--receive.hashrings-algorithm=hashmod
The algorithm used when distributing series in
the hashrings. Must be one of hashmod, ketama
the hashrings. Must be one of hashmod, ketama.
Will be overwritten by the tenant-specific
algorithm in the hashring config.
--receive.hashrings-file=<path>
Path to file that contains the hashring
configuration. A watcher is initialized
Expand Down
7 changes: 4 additions & 3 deletions pkg/receive/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@ const (
// HashringConfig represents the configuration for a hashring
// a receive node knows about.
type HashringConfig struct {
Hashring string `json:"hashring,omitempty"`
Tenants []string `json:"tenants,omitempty"`
Endpoints []string `json:"endpoints"`
Hashring string `json:"hashring,omitempty"`
Tenants []string `json:"tenants,omitempty"`
Endpoints []string `json:"endpoints"`
Algorithm HashringAlgorithm `json:"algorithm,omitempty"`
}

// ConfigWatcher is able to watch a file containing a hashring configuration
Expand Down
37 changes: 25 additions & 12 deletions pkg/receive/hashring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import (

"github.com/cespare/xxhash"

"github.com/go-kit/log"
"github.com/go-kit/log/level"

"github.com/pkg/errors"

"github.com/thanos-io/thanos/pkg/store/labelpb"
Expand Down Expand Up @@ -236,19 +239,14 @@ func newMultiHashring(algorithm HashringAlgorithm, replicationFactor uint64, cfg
cache: make(map[string]Hashring),
}

newHashring := func(endpoints []string) Hashring {
switch algorithm {
case AlgorithmHashmod:
return simpleHashring(endpoints)
case AlgorithmKetama:
return newKetamaHashring(endpoints, SectionsPerNode, replicationFactor)
default:
return simpleHashring(endpoints)
}
}

for _, h := range cfg {
m.hashrings = append(m.hashrings, newHashring(h.Endpoints))
var hashring Hashring
if h.Algorithm != "" {
hashring = newHashring(h.Algorithm, h.Endpoints, replicationFactor, h.Hashring, h.Tenants)
} else {
hashring = newHashring(algorithm, h.Endpoints, replicationFactor, h.Hashring, h.Tenants)
}
yeya24 marked this conversation as resolved.
Show resolved Hide resolved
m.hashrings = append(m.hashrings, hashring)
var t map[string]struct{}
if len(h.Tenants) != 0 {
t = make(map[string]struct{})
Expand Down Expand Up @@ -299,3 +297,18 @@ func HashringFromConfig(algorithm HashringAlgorithm, replicationFactor uint64, c

return newMultiHashring(algorithm, replicationFactor, config), err
}

func newHashring(algorithm HashringAlgorithm, endpoints []string, replicationFactor uint64, hashring string, tenants []string) Hashring {
switch algorithm {
case AlgorithmHashmod:
return simpleHashring(endpoints)
case AlgorithmKetama:
return newKetamaHashring(endpoints, SectionsPerNode, replicationFactor)
default:
l := log.NewNopLogger()
level.Warn(l).Log("msg", "Unrecognizable hashring algorithm. Fall back to hashmod algorithm.",
"hashring", hashring,
"tenants", tenants)
return simpleHashring(endpoints)
Copy link
Contributor

Choose a reason for hiding this comment

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

If an invalid algorithm is specified, do we just fallback to the hashmod algorithm?
I feel a better way is to either:

  1. Return non-exist error and let users know this is wrong.
  2. Fall back to the default one. But have a log to tell that the specified algorithm is wrong.

Those 2 options are better than ignoring the error.

Copy link
Contributor Author

@haanhvu haanhvu Sep 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@yeya24 I think Option 1 is better. Thought of that myself. But that would change the logic of the current newMultiHashring() a little bit. Would that cause a backward compatibility problem?

I see that we have an errors package at https://github.com/thanos-io/thanos/tree/main/pkg/errors but that doesn't seem to be used much? If we go with option 1, should I use that one or the github.com/pkg/errors one?

@fpetkovski @matej-g

Copy link
Collaborator

Choose a reason for hiding this comment

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

What would be the behavior though? Right now we do not return error as hashring creation (so to speak) "cannot" fail. Should we return error and ignore a hashring with invalid algorithm? Or how would we like to handle this?

This is why option 2 looked better, since we can just warn and fall back.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I look at this issue now, I feel either way is okay to me.
If we go with option 1, even if it is wrong, it is just one-off. If we go with option 2, it is also fine to me as this is better than current behavior.

}
}