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: Include current config hash in forward requests #3138

Closed
wants to merge 4 commits into from

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Sep 8, 2020

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

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

Changes

  • Add the hashring hash to the forward payload.

Verification

  • make test-local

// The replica value in the header is one-indexed, thus we need >.
if rep > h.options.ReplicationFactor {
return errBadReplica
}

if h.hashring.ConfigHash() != config {
return errHashringConfigMismatch
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not certain anymore whether erroring is really the right thing to do. We should always favor availability over consistency I think so we should rather accept the write but log or record in a metric that this has happened.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Alright. Make sense. It'd simplify things. I'm gonna add a metric and log this event.

Copy link
Member Author

@kakkoyun kakkoyun Sep 9, 2020

Choose a reason for hiding this comment

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

If we won't fail until we run mutlple multi-tsdb's per hashring configuration (as it suggested in #3141), the config hash is only going to be informative not actionable.

pkg/receive/handler.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun force-pushed the hashring_hash branch 2 times, most recently from ceff9cd to ad1fb2c Compare October 29, 2020 11:26
@kakkoyun kakkoyun changed the title [WIP] receive: Include current config hash in forward requests receive: Include current config hash in forward requests Oct 29, 2020
@kakkoyun kakkoyun marked this pull request as ready for review October 29, 2020 13:00
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Copy link
Member Author

kakkoyun commented Dec 10, 2020

@brancz Do you see any value to merge this? Or should I just close this one?

@brancz
Copy link
Member

brancz commented Dec 14, 2020

I think since we're splitting the architecture for the receive component, we can close this. Wdyt?

@kakkoyun
Copy link
Member Author

@brancz Sounds good to me 👍

@kakkoyun kakkoyun closed this Dec 16, 2020
@kakkoyun kakkoyun deleted the hashring_hash branch December 16, 2020 06:33
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 this pull request may close these issues.

2 participants