-
Notifications
You must be signed in to change notification settings - Fork 816
Remove remaining support for denormalised tokens in the ring. #2034
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
Remove remaining support for denormalised tokens in the ring. #2034
Conversation
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
…tex. Signed-off-by: Peter Štibraný <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, looks pretty straightforward
Signed-off-by: Peter Štibraný <[email protected]>
We don't expect to run a mix of old (with old fields) and new (with new field with the same number) Cortex versions in practice. Signed-off-by: Peter Štibraný <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Peter! The cleanup LGTM. I've also manually tested the ingester transfer. Would you mind taking a look at the few comments left, please?
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Thank you both for your reviews. I've addressed (hopefully all) your feedback.
Btw, we have integration test doing this already (it found some bugs in my code in the past). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR removes remaining support for denormalised tokens in the ring.
Targeted for post 0.6 release.
Reason for this change is to simplify the code. Nobody should be using denormalised tokens anymore, we have provided smooth upgrade path (post 0.4.0 Cortex only writes normalised tokens).
As we don't expect to run a mix of old (with old fields in ring protobuf) and new (with some new field with the same number) Cortex versions in practice, I've removed reserved fields from ring protobuf and also didn't reserve number for just-removed
Tokens
field.Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]