-
Notifications
You must be signed in to change notification settings - Fork 820
HA tracker limits #3668
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
HA tracker limits #3668
Conversation
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.
Good idea applying the label length limit to the replica too: LGTM! However, I left a couple of comments to the max cluster limit.
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 @pstibrany for addressing my feedback. Overall LGTM, but I left few last comments.
CHANGELOG.md
Outdated
@@ -39,6 +39,7 @@ | |||
* [CHANGE] Ruler: gRPC message size default limits on the Ruler-client side have changed: #3523 | |||
- limit for outgoing gRPC messages has changed from 2147483647 to 16777216 bytes | |||
- limit for incoming gRPC messages has changed from 4194304 to 104857600 bytes | |||
* [CHANGE] HA Tracker: verify label value limit for replica label. #3668 |
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.
* [CHANGE] HA Tracker: verify label value limit for replica label. #3668 | |
* [CHANGE] HA Tracker: verify label value length limit (`-validation.max-length-label-value`) for replica label. #3668 |
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.
Done. I've also moved entry to "master" section, somehow it was in 1.6 :)
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.
You did great! LGTM (I think the comment on CHANGELOG still makes sense, but up to you).
Oh, sorry I missed that! Thanks for review, I’ll fix the changelog before merging. |
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
…alidation fails. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
What this PR does: This PR verifies that replica label isn't larger than configured max label value limit, and also introduces new limit for maximum number of clusters tracked by HA tracker.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]