distribute_reads icon indicating copy to clipboard operation
distribute_reads copied to clipboard

Published 20 hours ago • verified publisher icon ankane

Specifying the name of the replica with the distribute_reads method

Open fabienpiette opened this issue 4 years ago • 6 comments

Issue: https://github.com/ankane/distribute_reads/issues/37

fabienpiette avatar May 26 '20 13:05 fabienpiette

Hey @fabienpiette, thanks for the PR, including tests 💯

As I was going reviewing/compiling feedback for this, I think I found a slightly different way that doesn't require a custom strategy (which would allow it to work with any strategy). I don't typically do this, but I put together a branch to try it out. I'd be curious to hear your thoughts. https://github.com/ankane/distribute_reads/compare/named_connections

For this PR, I believe the approach isn't thread-safe, as @slave_pool.current_name will be shared across threads.

All that being said, both approaches add a bit more complexity than I was hoping for, so still on the fence.

ankane avatar May 27 '20 01:05 ankane

Hi! Indeed the current_name wasn't thread safe, I find your approach much more elegant (especially since we don't have to define a new Makara strategy). I've tested it locally on my project and it seems to work well. It does add a bit of complexity but the code is clean and the code overload is only on the DistributeReads::Pool/Makara::Pool class and is quite minimal in my opinion. Anyway my original problem is solved, well done :+1:

fabienpiette avatar May 27 '20 08:05 fabienpiette

Great, glad it's working, and fwiw, your PR helped out a lot - was much easier to make some tweaks than start from scratch.

Going to leave this feature in a branch for now. Let me know if you run into any issues with it. Will feel more comfortable if it's running successfully in production for a while, as well as if others are interested in it.

ankane avatar May 28 '20 03:05 ankane

No problem, thanks to you :+1:

fabienpiette avatar May 28 '20 07:05 fabienpiette

Great, glad it's working, and fwiw, your PR helped out a lot - was much easier to make some tweaks than start from scratch.

Going to leave this feature in a branch for now. Let me know if you run into any issues with it. Will feel more comfortable if it's running successfully in production for a while, as well as if others are interested in it.

Are we merging this anytime sooner?

GaudamThiyagarajan avatar Jun 15 '20 06:06 GaudamThiyagarajan

Great, glad it's working, and fwiw, your PR helped out a lot - was much easier to make some tweaks than start from scratch. Going to leave this feature in a branch for now. Let me know if you run into any issues with it. Will feel more comfortable if it's running successfully in production for a while, as well as if others are interested in it.

Are we merging this anytime sooner?

Hey ! I've been able to test these modifications in production, no problems to report for the moment, I think it can be merged soon.

fabienpiette avatar Jun 15 '20 08:06 fabienpiette