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

Jump to bottom

Expose default configuration values for memberlist. #4276

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Expose default configuration values for memberlist. #4276

merged 4 commits into from
Jul 7, 2021

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented Jun 11, 2021 •
edited
Loading

What this PR does:
Set the defaults for various memberlist configuration values based on
the "Default LAN" configuration. The only result of this change is that
the defaults are now visible and are in the documentation. This also
means that if the default values change, then the changes are visible
in the documentation, where as before they would have gone unnoticed.

Note: The existing behaviour of a zero value is not retained. If a
configuration is explicitly set to zero, the default is not used.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@stevesg stevesg force-pushed the memberlist-cfg-defaults branch 2 times, most recently from 96b475c to c9b3359 Compare June 11, 2021 11:55
pstibrany
pstibrany reviewed Jun 14, 2021
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I'd suggest removing support for 0 values and simplify the code as a result.

pkg/ring/kv/memberlist/memberlist_client.go Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 21, 2021
@stevesg stevesg force-pushed the memberlist-cfg-defaults branch 2 times, most recently from 76ea0e0 to 2953383 Compare June 21, 2021 10:46
pstibrany
pstibrany approved these changes Jun 21, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pracucci
pracucci approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM! (modulo a nit related to the CHANGELOG entry)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@bboreham
Copy link
Contributor

Sorry but we have begun the process of cutting a new release; please rebase from master and move your CHANGELOG entry to the top under ## master / unreleased

@pstibrany pstibrany mentioned this pull request Jul 2, 2021
Closed
3 tasks
@alvinlin123
Copy link
Contributor

alvinlin123 commented Jul 2, 2021 •
edited
Loading

To prevent this being a breaking change, the existing behaviour is
retained, in case anyone is explicitly setting the values to zero and
expecting the default to be used.

Just a peanut gallery comment. Might be a good idea to remove the above from the PR description since we decided to remove zero value support. The reason I am suggesting this is so that people who come to this PR through Google would not get mislead to thinking there is still zero value support :-)

@stevesg stevesg force-pushed the memberlist-cfg-defaults branch from bbd34c7 to e691d2f Compare July 6, 2021 08:37
stevesg added 4 commits July 6, 2021 10:37
@stevesg
Expose default configuration values for memberlist.
11446ad 
Set the defaults for various memberlist configuration values based on
the "Default LAN" configuration. The only result of this change is that
the defaults are now visible and are in the documentation. This also
means that if the default values change, then the changes are visible
in the documentation, where as before they would have gone unnoticed.

To prevent this being a breaking change, the existing behaviour is
retained, in case anyone is explicitly setting the values to zero and
expecting the default to be used.

Signed-off-by: Steve Simpson <[email protected]>
@stevesg
Remove use of zero value as default value indicator.
47cf843 
Signed-off-by: Steve Simpson <[email protected]>
@stevesg
Review comments.
63742b4 
Signed-off-by: Steve Simpson <[email protected]>
@stevesg
Review comments.
61cabbb 
Signed-off-by: Steve Simpson <[email protected]>
@stevesg stevesg force-pushed the memberlist-cfg-defaults branch from e691d2f to 61cabbb Compare July 6, 2021 08:38
@stevesg
Copy link
Contributor Author

stevesg commented Jul 6, 2021

Just a peanut gallery comment. Might be a good idea to remove the above from the PR description since we decided to remove zero value support. The reason I am suggesting this is so that people who come to this PR through Google would not get mislead to thinking there is still zero value support :-)

Good spot, fixed! :)

@stevesg
Copy link
Contributor Author

stevesg commented Jul 6, 2021

This should be ready now; fell to the bottom of my priorities.

@pracucci pracucci merged commit f8b08a3 into cortexproject:master Jul 7, 2021
@bboreham
Copy link
Contributor

bboreham commented Aug 6, 2021

Sorry but we have begun the process of cutting a new release; please rebase from master and move your CHANGELOG entry to the top under ## master / unreleased

This wasn't done; I'll fix it in post.

alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
@stevesg @alvinlin123
Expose default configuration values for memberlist. (cortexproject#4276)
abcb297 
* Expose default configuration values for memberlist.

Set the defaults for various memberlist configuration values based on
the "Default LAN" configuration. The only result of this change is that
the defaults are now visible and are in the documentation. This also
means that if the default values change, then the changes are visible
in the documentation, where as before they would have gone unnoticed.

To prevent this being a breaking change, the existing behaviour is
retained, in case anyone is explicitly setting the values to zero and
expecting the default to be used.

Signed-off-by: Steve Simpson <[email protected]>

* Remove use of zero value as default value indicator.

Signed-off-by: Steve Simpson <[email protected]>

* Review comments.

Signed-off-by: Steve Simpson <[email protected]>

* Review comments.

Signed-off-by: Steve Simpson <[email protected]>
Signed-off-by: Alvin Lin <[email protected]>
@pracucci pracucci mentioned this pull request Feb 22, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bboreham bboreham bboreham left review comments

@pstibrany pstibrany pstibrany approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stevesg @bboreham @alvinlin123 @pstibrany @pracucci