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

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

Merged
merged 3 commits into from
Feb 7, 2023

Conversation

haanhvu
Copy link
Contributor

@haanhvu haanhvu commented Aug 28, 2022

Signed-off-by: haanhvu haanh6594@gmail.com

fixes #5567

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Receive: Allow setting hashing algorithm per tenant in hashrings config

Verification

@haanhvu haanhvu changed the title Issue5567 Receive: Allow setting hashing algorithm per tenant in hashrings config Aug 28, 2022
@haanhvu haanhvu marked this pull request as draft August 28, 2022 21:31
pkg/receive/hashring.go Outdated Show resolved Hide resolved
pkg/receive/hashring.go Outdated Show resolved Hide resolved
pkg/receive/hashring.go Outdated Show resolved Hide resolved
@haanhvu haanhvu marked this pull request as ready for review August 28, 2022 22:05
@haanhvu
Copy link
Contributor Author

haanhvu commented Aug 29, 2022

@fpetkovski could you pls review if this is what you mean in the issue?

fpetkovski
fpetkovski previously approved these changes Aug 29, 2022
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me 👍

@haanhvu haanhvu marked this pull request as ready for review August 30, 2022 15:26
case AlgorithmKetama:
return newKetamaHashring(endpoints, SectionsPerNode, replicationFactor)
default:
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

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.

matej-g
matej-g previously approved these changes Sep 9, 2022
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Would be good to address @yeya24's comment, otherwise looks good!

CHANGELOG.md Outdated
@@ -15,6 +15,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#5655](https://github.com/thanos-io/thanos/pull/5655) Receive: Fix recreating already pruned tenants.

### Added
- [#5653](https://github.com/thanos-io/thanos/pull/5653) Receive: Allow setting hashing algorithm per tenant in hashrings config
Copy link
Collaborator

@matej-g matej-g Sep 12, 2022

Choose a reason for hiding this comment

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

Nit: Can we specify that the hashring config from file has precedence over the flag?

Copy link
Contributor Author

@haanhvu haanhvu Sep 13, 2022

Choose a reason for hiding this comment

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

Yeah I'm thinking we can have a log telling where we take the algorithm from.

Also, we need to edit the receive.md doc and the description of receive.hashrings-algorithm flag (anything else?)

Where can I edit the description of the receive.hashrings-algorithm flag?

Copy link
Collaborator

@matej-g matej-g Sep 13, 2022

Choose a reason for hiding this comment

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

In here: https://github.com/thanos-io/thanos/blob/main/cmd/thanos/receive.go#L820, I believe the .md is automatically generated based on this description

@haanhvu
Copy link
Contributor Author

haanhvu commented Sep 13, 2022

Just thought of this case, currently if users specify the algorithm as Ketama, not ketama, we would consider it invalid and fall back to hashmod?

AlgorithmHashmod HashringAlgorithm = "hashmod"
AlgorithmKetama HashringAlgorithm = "ketama"

@matej-g @fpetkovski @yeya24

@matej-g
Copy link
Collaborator

matej-g commented Sep 13, 2022

Just thought of this case, currently if users specify the algorithm as Ketama, not ketama, we would consider it invalid and fall back to hashmod?

Preferably we should convert the string to lowercase (if this is not already done by the library we use for flags?) and accept it, more user friendly.

@fpetkovski
Copy link
Contributor

fpetkovski commented Sep 13, 2022

I am not sure I like where this rabbit hole is taking us :)

What about adding validation here so that we can take the guesswork out: https://github.com/thanos-io/thanos/blob/main/pkg/receive/hashring.go#L289? That should resolve @yeya24's comment if I'm not mistaken.

@haanhvu
Copy link
Contributor Author

haanhvu commented Sep 25, 2022

I made all the required changes. There're still some failed checks because I'm having a problem with Makefile (go version problem). Will look into it tomorrow.

@@ -296,6 +292,21 @@ func HashringFromConfig(algorithm HashringAlgorithm, replicationFactor uint64, c
if len(config) == 0 {
return nil, errors.Wrapf(err, "failed to load configuration")
}

Copy link

Choose a reason for hiding this comment

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

gofmt: File is not gofmt-ed with -s


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -296,6 +292,21 @@ func HashringFromConfig(algorithm HashringAlgorithm, replicationFactor uint64, c
if len(config) == 0 {
return nil, errors.Wrapf(err, "failed to load configuration")
}

if (algorithm != "hashmod") && (algorithm != "ketama") {
log.Println("The specified algorithm is incorrect. Fall back to hashmod algorithm")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we log more information about the hashring to help debugging & problem solving? For example, the hashring name and tenants could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution, @haanhvu 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@douglascamata Do you think log.Println is good enough for all cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I don't think so, @haanhvu. It's better to use our chosen logging library ("github.com/go-kit/log"), which might require that you send down the logger as a parameter to this function and then do something like:

# import "github.com/go-kit/log/level"
level.Warning(logger).Log(...)

I think Warning is the best level as this doesn't stop Receive from starting up.

@matej-g
Copy link
Collaborator

matej-g commented Oct 27, 2022

@haanhvu any updates on this one?

@haanhvu
Copy link
Contributor Author

haanhvu commented Oct 27, 2022

@matej-g There're some small fixes left to do (mostly the log as @douglascamata suggested). But I need to finish #5777 first before getting back to this. Can't be in two rabbit holes at the same time...

@matej-g
Copy link
Collaborator

matej-g commented Oct 27, 2022

No worries @haanhvu, I was just sifting through PRs and wanted to know latest status, carry on and thank you 😊

@haanhvu
Copy link
Contributor Author

haanhvu commented Jan 3, 2023

If an invalid algorithm is specified, do we just fallback to the hashmod algorithm?
I feel a better way is to either:
Return non-exist error and let users know this is wrong.
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.

Can we log more information about the hashring to help debugging & problem solving? For example, the hashring name and tenants could be useful.

@matej-g @douglascamata @yeya24 Hey about the log, I'm not sure where to put it for now. In #5777 we edited and deleted a lot of functions in hashring.go. If these changes are merged (which will be merged soon, I think), maybe I'll need to put the hashring algorithm checking log to receive cmd or config.go, instead of hashring.go like currently.

So I think we can add the log later, after #5777 merged. Is that ok?

@douglascamata
Copy link
Contributor

Sounds good to me. Not sure what the maintainers think.

Signed-off-by: haanhvu <haanh6594@gmail.com>
Signed-off-by: haanhvu <haanh6594@gmail.com>
Signed-off-by: haanhvu <haanh6594@gmail.com>
@haanhvu haanhvu marked this pull request as ready for review February 5, 2023 19:41
@haanhvu
Copy link
Contributor Author

haanhvu commented Feb 5, 2023

I made all the required changes. Please help review!

@haanhvu haanhvu requested review from douglascamata and yeya24 and removed request for douglascamata and yeya24 February 5, 2023 19:42
Copy link
Contributor

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

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

Neat and clean. LGTM! 🚀

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@yeya24 yeya24 merged commit 4bb560e into thanos-io:main Feb 7, 2023
@haanhvu
Copy link
Contributor Author

haanhvu commented Feb 9, 2023

I completely forgot. Do we need to add test? Like a simple test that shows the algorithm in hashring config will overwrite the global algorithm?

@douglascamata
Copy link
Contributor

@haanhvu tests are always welcome! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Receive: Allow setting hashing algorithm per tenant in hashrings config
5 participants