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

Loki: Add route_randomly to Redis options #8852

Merged
merged 7 commits into from Mar 22, 2023
Merged

Loki: Add route_randomly to Redis options #8852

merged 7 commits into from Mar 22, 2023

Conversation

wtchangdm
Copy link
Contributor

@wtchangdm wtchangdm commented Mar 21, 2023

What this PR does / why we need it:

Current Redis client only reads from master node, which makes master node under heavy pressure.

Usually, one or more read replicas are expected on production environments.

This PR helps to increase the utilization of read replicas.

Which issue(s) this PR fixes:
Fixes #8811

Special notes for your reviewer:

  • The route_randomly option defaults to false, which matches the same behavior in previous versions, hence no breaking change.
  • User needs to explicitly set route_randomly to true in order to use it.
  • The purpose of this PR is to evenly distribute the read requests, so only RouteRandomly is used and not the following:
    • ReadOnly (which only reads from read replicas)
    • RouteByLatency (requests probably won't be even since not all Loki components are deployed evenly separate)

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@wtchangdm wtchangdm requested review from JStickler and a team as code owners March 21, 2023 12:29
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 21, 2023
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, please just add some more documentation
Thanks for the contribution @wtchangdm!

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Thanks! I touched up the wording a little.
Please merge main into your branch and rerun make doc and commit the result.

pkg/storage/chunk/cache/redis_client.go Outdated Show resolved Hide resolved
@wtchangdm
Copy link
Contributor Author

Thanks @dannykopping, I've regenerated the doc and rebased on main.

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

@wtchangdm
Copy link
Contributor Author

Thanks again @dannykopping!

@dannykopping dannykopping merged commit 183fe85 into grafana:main Mar 22, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Redis config to set reading across all replicas
2 participants