albrow / jobs

A persistent and flexible background jobs library for go.
MIT License
499 stars 47 forks source link

Support redis sentinel #15

Open epelc opened 9 years ago

epelc commented 9 years ago

@albrow Have you thought about adding sentinel support? I see you use the redigo package which is what I also use. But It hasn't been updated much lately(last commit was 3 months one before was 6 months old).

I was thinking about switching to go-redis/redis which has built in sentinel support and I think better connection pooling support. As you can see it's a very active project and they are also about to release v3 which has an even nicer api.

I know redigo has a fork which seems to support sentinel but to me it really seems like a dead project.

albrow commented 9 years ago

@epelc, thank you for letting me know about this. Sentinel support is something I can try and include if needed.

When I was first writing zoom I tried out several different redis drivers before settling on redigo. I have been very very happy with it so it would take a lot to convince me to switch.

If sentinel support is important to you and/or other users, I would prefer switching to hbcheng's fork in order to provide that. I'd also probably chime in on garyburd/redigo to see if we could get it merged into master.

go-redis/redis looks okay at first glance. I've been looking over the source code, in particular to see how they manage connection pools and pipelined commands. The support for scripts is not as good as in redigo, which handles the EVALSHA semantics for you and just lets you call Do. I also have some concerns about thread-safety on go-redis and opened an issue asking for clarification.

@epelc are you working on an application that requires sentinel support? Or is anyone else? If so I'll make this issue a higher priority.

epelc commented 9 years ago

@albrow I'd be happy if we could get the sentinel fork merged back into redigo. I was looking over the commit history and it has been actively developed. I also prefer the strings as commands approach which is like mgo.

Heres some background why I asked for this. I currently run a 3 node master slave redis setup with 2 slaves. They all run in docker with --restart=always so they auto restart if they crash or if the machine reboots/start stops etc. But I had an issue with how I mounted the config file for redis there was a race condition. This lead to my master dying anytime the machine rebooted(but not if redis crashed).

Anyways I fixed the race condition and it handles restarts fine now but I'd like to have a more HA type setup and sentinel seems prod ready now. Essentially redis has become the weak point of my stack(everything else is HA with 2 failover nodes) if my redis master blows up I need manual intervention(everything else I can have 1-2 masters blow up).

I'll probably switch my app over to the fork you mentioned and test it out. But I'd be much happier if jobs supported sentinel too.

albrow commented 9 years ago

@epelc Thanks for sharing your setup. I want to implement sentinel support for you and anyone else who needs it, but it's going to take some time. Just so we're on the same page.. I want to layout the requirements for sentinel support:

  1. Configuration should take a list of master addresses instead of a single address. (Should jobs write to all masters?)
  2. In the event of a failover (i.e. when a master fails and a slave gets promoted to the new master), writes that would have been sent to the old master should be sent to the new master.

Is this what you had in mind? Am I missing anything?

epelc commented 9 years ago

@albrow I don't think I was clear. I have 1 master and 2 slaves. If I used sentinel then say my master blows up sentinel would turn one of my slaves into the new master. Then the old master if it ever came back would become a slave.

I think the only time you have multiple masters is if your using redis cluster but I don't know much about that. I'd just change the list of masters to a list of servers. I'm also not sure if you configure the redis client with the list of redis servers or the list of sentinel servers I'll have to look into this. I'll get back to you after I try out that fork you mentioned.

albrow commented 9 years ago

Okay thanks. I think I understand the single master, multiple slaves use case pretty well. The reason I asked about multiple masters is that this API documentation mentions the SENTINEL masters command which returns a list of masters. I think maybe multiple masters is another redundancy technique? Maybe a less common one. Or maybe the multiple masters are supposed to be independent and not share data.

epelc commented 9 years ago

I think it's for redis cluster as I'm pretty sure they use a master and several slaves for each group of ids(they break your keys into groups to spread out over your cluster). I think you would use the SENTINEL slaves masterName command to get a list of slaves for the master you give it.