etaty / rediscala

Non-blocking, Reactive Redis driver for Scala (with Sentinel support)
Apache License 2.0
790 stars 142 forks source link

Add an optional db number parameter to SentinelMonitoredRedisClient a… #127

Closed mmagn closed 8 years ago

mmagn commented 8 years ago

…nd SentinelMonitoredRedisBlockingClient

related to this issue : https://github.com/etaty/rediscala/issues/126

etaty commented 8 years ago

I think it would be better to just expose a function createClient: (String, Int) => RedisClient = (ip, port) => new RedisClient(ip, port, name = "SMRedisClient" as the parameter of SentinelMonitoredRedisClient as the next guy will ask for changing the name or be able to set the password, or some other crazy stuff.

etaty commented 8 years ago
case class SentinelMonitoredRedisClient( sentinels: Seq[(String, Int)] = Seq(("localhost", 26379)),
                                         master: String,
                                         createClient: (String, Int) => RedisClient = (ip, port) => new RedisClient(ip, port, name = "SMRedisClient")
                                       (implicit system: ActorSystem,
                                        redisDispatcher: RedisDispatcher = Redis.dispatcher
                                        ) extends SentinelMonitoredRedisClientLike(system, redisDispatcher) with RedisCommands with Transactions {

  val redisClient: RedisClient = withMasterAddr(createClient)
  override val onNewSlave  =  (ip: String, port: Int) => {}
  override val onSlaveDown =  (ip: String, port: Int) => {}
}
mmagn commented 8 years ago

I tried to expose the createClient function as a parameter like in your example, but the compiler throws me an error :

could not find implicit value for parameter _system: akka.actor.ActorSystem
Error occurred in an application involving default arguments.

The problem is that the RedisClient instantiation in default function parameter needs a implicit ActorSystem.

Do you have any idea to make it right ?

etaty commented 8 years ago

I will try to have a look at it this weekend. I think the only way is to separate the config (RedisConfig) from the implicits (system and dispatcher), but i would require a large refactoring...

mmagn commented 8 years ago

If you don't want to go into the large refactoring, I can add to my first commit, password and name parameters. It will do the job for a first version.

etaty commented 8 years ago

yeap we can do that, thanks

mmagn commented 8 years ago

One of my build task failed, I saw you already encountered this problem, is there a build stability issue ?

etaty commented 8 years ago

Yep It happens with travis, probably related to some timing, as travis cpu are really slow.

mmagn commented 8 years ago

Ok thanks.

etaty commented 8 years ago

snapshot available

resolvers += "Sonatype OSS Snapshots" at "https://oss.sonatype.org/content/repositories/snapshots"
libraryDependencies += "com.github.etaty" %% "rediscala" % "1.7.0-SNAPSHOT"