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

Make elan read from redis (and a bunch of other improvements to redis). #307

Merged
merged 8 commits into from
May 2, 2024

Conversation

fische
Copy link
Contributor

@fische fische commented May 2, 2024

This PR makes elan read from Redis before hitting the bucket, so we can save on bucket operations. It does not make it write to redis as we were worried it would highly increase the number of evicted keys. We might experiment with that in the future.

There are a bunch of additional improvements to the redis config as well:

  • to avoid maintaining 2 separate sets of flags, I've moved the redis options from the mettle worker package to //redis;
  • to avoid reimplementing the error rate limiter we use in the worker code, I implemented the Limiter interface from the redis package, so it is now supported natively within the clients;
  • I've also made the redis MaxSize configurable, as we might want to tweak it at some point to make sure we use Redis to its full potential.

Those additional changes came in as I faced the issues they are trying to address. They are completely separate from the elan change and could be moved to a separate PR. If you think this PR is too big, I'm happy to split it.

@fische fische self-assigned this May 2, 2024
mettle/worker/redis.go Outdated Show resolved Hide resolved
Hamishpk
Hamishpk previously approved these changes May 2, 2024
@fische fische merged commit f31b091 into thought-machine:master May 2, 2024
5 checks passed
@fische fische deleted the elan-redis branch May 2, 2024 14:58
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.

2 participants