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

Sentinel support #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sentinel support #79

wants to merge 1 commit into from

Conversation

univerio
Copy link

I added a sentinel client, plus dynamic master address support for the normal clients. There's a tiny breaking API change, due to the default parameters on RedisClientPool and the fact that scala apparently only supports default parameters on the primary constructor.

My bootstrap script to make testing easier:

#!/usr/bin/env bash

set -e
trap 'kill $PID0 $PID1 $PID2 $PID3 $PID4 $PID5 $PID6 $PID7 > /dev/null 2>&1' EXIT

function start_node {
    redis-server --port $1 --save "" > /dev/null 2>&1 &
}

function start_sentinel {
    redis-server --sentinel --port $1 --sentinel monitor scala-redis-test localhost 6379 2 > /dev/null 2>&1 &
}

start_node 6379
PID0=$!
start_node 6380
PID1=$!
start_node 6381
PID2=$!
start_node 6382
PID3=$!
start_sentinel 26379
PID4=$!
start_sentinel 26380
PID5=$!
start_sentinel 26381
PID6=$!
start_sentinel 26382
PID7=$!

sbt test

@@ -146,6 +155,12 @@ private [redis] trait R extends Reply {
case _ => Iterator.single(None)
}.toList)

def asListOfListPairs[A,B](implicit parseA: Parse[A], parseB: Parse[B]): Option[List[Option[List[Option[(A,B)]]]]] =
receive(multiMultiBulkReply).map(_.map(_.map(_.grouped(2).map {

Choose a reason for hiding this comment

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

throw new MapOverflowException :)

Copy link
Author

Choose a reason for hiding this comment

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

Heh, will probably need refactoring if it gets to ListOfListOfListPairs.

@debasishg
Copy link
Owner

Thanks for the PR. Will take a detailed look and merge. Apologies for the delay, awfully busy at this point in time :-(

@elyzion
Copy link

elyzion commented Oct 23, 2013

I am looking to use sentinel functionality if possible, does it look like this will be merged into the master anytime soon?

@debasishg
Copy link
Owner

Sorry .. very sorry .. I just didn't have time to look into it. Will have a look very soon and merge ..

@elyzion
Copy link

elyzion commented Oct 24, 2013

No problems, I can help test it after the merge as well.

@debasishg
Copy link
Owner

Merged sentinel support in branch scala-redis-sentinel https://github.com/debasishg/scala-redis/tree/scala-redis-sentinel .. You want to give it a spin. I ran using the bootstrap script sentinel.sh and found all tests running.

@elyzion
Copy link

elyzion commented Oct 28, 2013

Thank you! Will be checking it out today :)

@elyzion
Copy link

elyzion commented Oct 31, 2013

It seems to be working fine in my system!

However, there is still a behavioural issue; when the master node fails for a RedisClientPool, there is a period of time in which a "ConnectException" is returned. One would expect this to roughly correspond to the failover configuration for the sentinel system, but this is not the case. For a failover time of 5 seconds, one needs to wait roughly 20 seconds before one can use the RedisClientPool again without a ConnectException being raised. (Then again, I might be misunderstanding something somewhere, and this might be the correct behaviour.)

I'll try and take a look at it if I have the energy/time, but it's fairly low priority for me as everything else seems to be working fine.

@leonardschneider
Copy link

So this PR is dead?

@debasishg
Copy link
Owner

I did not merge this with master since there were breaking changes for the client. It was in a separate branch as I mentioned in a comment above. It was not tested much and I guess was not used much as well. I did some brief testing and found a few issues which need to be fixed. However I could not find time to pursue on this track any further. If there is really demand for using sentinel I can take this up seriously, though I am not sure about the time commitment. Still I would like to continue it as a separate branch. The effort required now is to backport the changes from master to this branch. As usual PRs are welcome, otherwise I will try to get this up and running in my spare time.

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.

5 participants