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

Add multi sentinel support #11

Merged
merged 5 commits into from
Oct 7, 2013
Merged

Conversation

tovbinm
Copy link
Contributor

@tovbinm tovbinm commented Sep 9, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fe6db9c on tovbinm:mt/multi_sentinel into * on etaty:master*.

@@ -177,7 +176,7 @@ abstract class SentinelMonitored(system: ActorSystem) {
def withMasterAddr[T](initFunction: (String, Int) => T): T = {
import scala.concurrent.duration._

val f = sentinelClient.getMasterAddr(master) map {
val f = sentinelClients.head.getMasterAddr(master) map {
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be stupid to ask the first Sentinel, then if it fails, try the second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right! We can do something like:

val f = sentinelClients.map(_.getMasterAddr(master))
val ff = Future.find(f) { case Some(x) => true case _ => false }
               .map {
                      case Some(Some((ip: String, port: Int))) => initFunction(ip, port)
                      case _ => throw new Exception(s"No such master '$master'")
                    }

Await.result(ff, 15 seconds)

Copy link
Owner

Choose a reason for hiding this comment

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

I think firstCompletedOf doesn't work if the first reply from Redis says "no 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.

Yeah, I came to the same conclusion ;)

@etaty
Copy link
Owner

etaty commented Sep 9, 2013

Really great :)

I don't really know about the use cases of Sentinel.
Maybe it would be nice to add and remove Sentinels when they spawn and die.
So during the life of you Scala program you could start with a cluster of A machines and move to B machines.

@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 9, 2013

Usually you run one sentinel process on each machine. For instance, if we have three Redis instances: one master and two slaves; then we'll have three sentinel processes, one per machine. Sentinels are not likely to die, except when the whole machine is down.

@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 9, 2013

Oh, do you mean detecting new sentinels automatically? i.e. subscribing to "+sentinel " and "-dup-sentinel " ?

@etaty
Copy link
Owner

etaty commented Sep 9, 2013

Yes, detecting new sentinels and starting a connection to it.

@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 9, 2013

Then it probably should be done internally in SentinelClient.

@etaty
Copy link
Owner

etaty commented Sep 9, 2013

Yes SentinelClient fire a callback, and the MonitoredClient can act on it (starting a new SentinelClient for example)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 731948a on tovbinm:mt/multi_sentinel into * on etaty:master*.

…nd update it when sentinel processes go up and down, by subcribing to '+sentinel' and '+sdown'
@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 10, 2013

@etaty , added sentinels auto discovery

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5cdd8b4 on tovbinm:mt/multi_sentinel into * on etaty:master*.

@etaty
Copy link
Owner

etaty commented Sep 11, 2013

looks good, i will merge it during the weekend.
thanks :)

@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 11, 2013

Great! Looking forward hearing from you.

def onNewSentinel(masterName: String, sentinelip: String, sentinelport: Int) = {
val k = makeSentinelClientKey(sentinelip, sentinelport)
if (master == masterName && !sentinelClients.contains(k)) {
synchronized {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe sentinelClients.synchronized will be better (sync only on the Map instance, instead of SentinelMonitored)
(it's just a protection for the future)

Copy link
Owner

Choose a reason for hiding this comment

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

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 prefer sentinelClients.synchronized, since using SynchronizedMap will not prevent from double addition/removal from the map. For this purpose we have the if (!sentinelClients.contains(k)) statement inside the synchronized section.

@etaty
Copy link
Owner

etaty commented Sep 15, 2013

Can you tell me if it's ready to be merged ?

@tovbinm
Copy link
Contributor Author

tovbinm commented Sep 15, 2013

Not yet. I'll ping you. ;)

-Matthew
On Sep 16, 2013 12:38 AM, "Valerian" notifications@github.com wrote:

Can you tell me if it's ready to be merged ?


Reply to this email directly or view it on GitHubhttps://github.com//pull/11#issuecomment-24480891
.

@tovbinm
Copy link
Contributor Author

tovbinm commented Oct 3, 2013

@etaty what else should be done for this one?

@etaty
Copy link
Owner

etaty commented Oct 3, 2013

Merging :)
I will do it during this weekend

@tovbinm
Copy link
Contributor Author

tovbinm commented Oct 3, 2013

Awesome!

@etaty etaty merged commit 887a674 into etaty:master Oct 7, 2013
@etaty
Copy link
Owner

etaty commented Oct 7, 2013

thanks :)

I will release version 1.3 during the week

# 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.

3 participants