Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Sentinel pool slave #103

Merged
merged 4 commits into from
Jan 2, 2016
Merged

Sentinel pool slave #103

merged 4 commits into from
Jan 2, 2016

Conversation

npeters
Copy link
Contributor

@npeters npeters commented Dec 5, 2015

sentinel manage a slave pool:

  • connect/disconnect
  • load balancing

@etaty
Copy link
Owner

etaty commented Dec 5, 2015

👍
Ping me when it is ready for review

@npeters
Copy link
Contributor Author

npeters commented Dec 8, 2015

today ?

def redisConnection: ActorRef = masterClient.redisConnection

override def send[T](redisCommand: RedisCommand[_ <: RedisReply, T]): Future[T] = {
if (redisCommand.isMasterOnly || slavesClients.getConnectionsActive.isEmpty) {
Copy link
Owner

Choose a reason for hiding this comment

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

getConnectionsActive is expensive (even more for just a isEmpty)
I think we can cache the result of getConnectionsActive or something like that
I also think most of the time we only need 1 active connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will replace by this

if (redisCommand.isMasterOnly || slavesClients.redisServers.exists(_.active.single.get)) {
      masterClient.send(redisCommand)
    } else {
      slavesClients.send(redisCommand)
    }

Copy link
Owner

Choose a reason for hiding this comment

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

yeah i think we can be even smarter trying to mix in the getNextConnection in RoundRobinPoolRequest

The ideal would be to find the next active connection in the RoundRobinPoolRequest
If we find it, then we have a slave where we can send the command. Otherwise we can send the command to the master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it exactly what happens: slavesClients is a RoundRobinPoolRequest

@etaty
Copy link
Owner

etaty commented Dec 8, 2015

Can you put the return for all the function even : Unit = {}

lazy val redisMasterSlavesPool =
SentinelMonitoredRedisClientMasterSlaves( master = masterName,
sentinels = sentinelPorts.map((redisHost, _)))
"sentienl slave pool" should {
Copy link
Owner

Choose a reason for hiding this comment

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

sentienl 😜

@etaty
Copy link
Owner

etaty commented Dec 8, 2015

Hey, you are on the good path 😃
ps: You need to be careful with the indentation (sometime there is too much tabs or not enough)
(I will try to automate that #107)

@npeters npeters force-pushed the sentinel-pool-slave branch from 87455b9 to c7de528 Compare December 9, 2015 01:25
@npeters npeters force-pushed the sentinel-pool-slave branch from c7de528 to 762ecad Compare December 9, 2015 01:55
@@ -35,13 +30,12 @@ abstract class RedisClientPoolLike(system: ActorRefFactory) extends RoundRobinPo
}
}

val redisConnectionRef: Ref[Seq[ActorRef]] = Ref(getConnectionsActive)
lazy val redisConnectionRef: Ref[Seq[ActorRef]] = Ref(getConnectionsActive)
Copy link
Owner

Choose a reason for hiding this comment

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

val redisConnectionRef: Ref[Seq[ActorRef]] = Ref(Seq.empty)
When we start we have no slave

Copy link
Owner

Choose a reason for hiding this comment

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

Okay I was wrong,
but can you remove the lazy

@etaty
Copy link
Owner

etaty commented Dec 9, 2015

In the tests there are a lot of Thread.sleep and println, could you try to clean that?
For example, you could use within(1 second, 4 seconds), I use that in redis.actors test

@npeters
Copy link
Contributor Author

npeters commented Dec 14, 2015

I finally find a bug on mutable pool
I try to simplify the design

@etaty etaty merged commit 3bc33e9 into etaty:master Jan 2, 2016
@etaty
Copy link
Owner

etaty commented Jan 2, 2016

thanks

@etaty etaty added this to the 1.6.0 milestone Jan 3, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants