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

Add read_from_replicas method and environment variable support for Redis cluster configuration #351

Conversation

fruscianteee
Copy link
Contributor

Overview

This PR introduces the following changes:

  1. Added the read_from_replicas method to the Config struct for easier configuration.
    • The read_from_replicas method allows enabling read operations from Redis replica nodes in a more intuitive way.
  2. Enabled reading the REDIS_CLUSTER__READ_FROM_REPLICAS=true environment variable for configuration.
    • The configuration now supports environment variables such as REDIS_CLUSTER__READ_FROM_REPLICAS for enabling or disabling reading from replicas.

Changes

  • Implemented the read_from_replicas method to simplify the process of enabling replica reads.
  • Updated the configuration handling to support reading environment variables, specifically REDIS_CLUSTER__READ_FROM_REPLICAS.

Testing

  • Added a test to ensure that Redis write operations work correctly when read_from_replicas is enabled.
  • Skipped testing whether read commands are actually sent to replicas, as this behavior is managed internally by redis-rs, and we rely on its implementation for this verification.

Issue

This PR addresses issue #350.

@bikeshedder
Copy link
Owner

Could you please look into the clippy error and remove the superfluous #[must_use]. Actually I would prefer for the read_from_replicas method to be removed. Writing cfg.read_from_replicas = true is just as easy as calling a cfg.read_from_replicas() method.

@bikeshedder
Copy link
Owner

Is there a reason to keep the read_from_replicas() method? I'd rather remove it if it doesn't add any value. It just adds multiple ways to do the same thing.

@fruscianteee
Copy link
Contributor Author

Thank you for your comment. The reason I implemented the read_from_replicas method was that I thought it might be easier to understand if the enabling method worked similarly to how it's done in redis-rs. However, as you pointed out, it would be better to keep things simpler by removing the method. I will proceed with that change.

@fruscianteee
Copy link
Contributor Author

(I made the must_use change before receiving your comment, which might have caused some confusion, making it seem like I wouldn't address the read_from_replicas change. I apologize for that.)

@bikeshedder
Copy link
Owner

Could you also add this to the CHANGELOG.md of the deadpool-redis crate? That'd be awesome. 🥳

I was about to make a deadpool-redis release. As this changes a public structure it needs to be a "breaking release" even though I don't think anyone uses the cluster config directly.

@fruscianteee
Copy link
Contributor Author

OK! I will also add the changes to the CHANGELOG.md, so please wait a moment.

@bikeshedder
Copy link
Owner

You changed the r2d2/CHANGELOG.md by accident. That should've been added to the redis/CHANGELOG.md instead. 🙈

@fruscianteee
Copy link
Contributor Author

Oh no! I’m sorry! I’ve fixed it now!

@bikeshedder bikeshedder merged commit b559365 into bikeshedder:master Sep 9, 2024
64 checks passed
@fruscianteee fruscianteee deleted the feature/add-read-from-replicas-method branch September 9, 2024 14:32
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