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

Remove deprecation in redis jsonnet #4640

Merged
merged 6 commits into from
Apr 4, 2023
Merged

Remove deprecation in redis jsonnet #4640

merged 6 commits into from
Apr 4, 2023

Conversation

lamida
Copy link
Contributor

@lamida lamida commented Mar 31, 2023

What this PR does

Update #4386 with deprecation removal and detail of changed configs.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
@lamida lamida requested a review from pracucci March 31, 2023 13:12
@lamida lamida marked this pull request as ready for review March 31, 2023 13:13
@lamida lamida requested a review from a team as a code owner March 31, 2023 13:13
Copy link
Collaborator

@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 thanks! I just want to wait for @56quarters to see if he has any objection.

@@ -88,7 +88,17 @@

* [CHANGE] Ruler: changed ruler deployment max surge from `0` to `50%`, and max unavailable from `1` to `0`. #4381
* [CHANGE] Memcached connections parameters `-blocks-storage.bucket-store.index-cache.memcached.max-idle-connections`, `-blocks-storage.bucket-store.chunks-cache.memcached.max-idle-connections` and `-blocks-storage.bucket-store.metadata-cache.memcached.max-idle-connections` settings are now configured based on `max-get-multi-concurrency` and `max-async-concurrency`. #4591
* [CHANGE] Add support to use external Redis as cache. #4386
* [CHANGE] Add support to use external Redis as cache. Following are some changes in the jsonnet config: #4386 #4640
Copy link
Contributor

Choose a reason for hiding this comment

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

The selection of settings mentioned here seems random (some enabled flags but not others, some item size flags but not others). Can we say that the various enabled flags have been renamed and the backend flags have been added since that a bit more accurately describes the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to be more specific about the exact changes that we did. Can reordering the items in the bullet list help? If not I will apply your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the changelogs detail as suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous version of the CHANGELOG was offering better guidance. What if we say something like this:

  • Renamed memcached_*_enabled config options to cache_*_enabled
  • Renamed memcached_*_max_item_size_mb config options to cache_*_max_item_size_mb
  • Added cache_*_backend config options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with latest @pracucci suggestion.

Signed-off-by: Jon Kartago Lamida <me@lamida.net>
@lamida lamida requested a review from 56quarters April 3, 2023 08:15
Signed-off-by: Jon Kartago Lamida <me@lamida.net>
Copy link
Collaborator

@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.

Thanks!

@pracucci pracucci merged commit 6563ba6 into main Apr 4, 2023
@pracucci pracucci deleted the lamida/jsonnet-redis2 branch April 4, 2023 08:38
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.

3 participants