bikeshedder / deadpool

Dead simple pool implementation for rust with async-await
Apache License 2.0
1.08k stars 137 forks source link

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

Closed fruscianteee closed 2 months ago

fruscianteee commented 2 months ago

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

Testing

Issue

This PR addresses issue #350.

bikeshedder commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

(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 commented 2 months ago

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

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 commented 2 months ago

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

bikeshedder commented 2 months ago

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

fruscianteee commented 2 months ago

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