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

Support a clustered redis #1215

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Support a clustered redis #1215

merged 4 commits into from
Oct 31, 2023

Conversation

sjahl
Copy link
Contributor

@sjahl sjahl commented Oct 27, 2023

works on #1183 -- probably need a developer to vet this a bit...

@sjahl sjahl self-assigned this Oct 27, 2023
@sjahl sjahl linked an issue Oct 27, 2023 that may be closed by this pull request
Create Redis cluster ahead of ASHG #1183
Closed
@sjahl
Copy link
Contributor Author

sjahl commented Oct 27, 2023

OK, testing this in a dev environment, and things seem to be working mostly as expected.

Given the time crunch, I don't know if this is a now thing, or a later thing... but right now this config ONLY supports a clustered redis with a failover sentinel. It would be nice for us to maybe handle the use case where someone can easily drop in a single redis node instead. Could also lob that into the project to fix how we handle not having a redis in #1208

phildarnowsky-broad
phildarnowsky-broad approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@phildarnowsky-broad phildarnowsky-broad left a comment

Choose a reason for hiding this comment

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

I pretty much have to take the k8s changes on faith but everything you did in the frontend code looks legit. I just have some small suggestions regarding squashing this down to a smaller number of commits, no notes otherwise.

deploy/manifests/browser/base/api.deployment.yaml Outdated
@@ -43,16 +43,16 @@ spec:
key: ips
# Global rate limiting
- name: MAX_CONCURRENT_ES_REQUESTS
value: '100'
value: '10'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it looks like this commit just undoes changes made in the immediately previous commit, let's merge this one into the previous one. I made Elissa do it, fair's fair. 😅

deploy/manifests/browser/base/api.deployment.yaml Outdated
@@ -35,7 +35,7 @@ spec:
- name: WRITE_CACHE_REDIS_URL
value: redis
- name: RATE_LIMITER_REDIS_URL
value: redis://redis:26379/2
value: redis
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you could merge this commit and the previous two all together.

@sjahl sjahl marked this pull request as ready for review October 30, 2023 20:03
@sjahl sjahl changed the base branch from main to gnomad-v4 October 31, 2023 16:12
@sjahl sjahl merged commit 8a4c959 into gnomad-v4 Oct 31, 2023
@rileyhgrant rileyhgrant deleted the clustered-redis branch January 17, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phildarnowsky-broad phildarnowsky-broad phildarnowsky-broad approved these changes

Assignees

@sjahl sjahl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Redis cluster ahead of ASHG
2 participants
@sjahl @phildarnowsky-broad