debasishg / scala-redis

A scala library for connecting to a redis server, or a cluster of redis nodes using consistent hashing on the client side.
1.02k stars 219 forks source link

adding all settings available to Cluster clients #251

Closed atais closed 4 years ago

atais commented 5 years ago

Optons:

were missing for cluster config

@debasishg do you plan to release new version soon?

debasishg commented 4 years ago

@atais I was off to some other stuff for the last month or so. Could not touchbase on this. Apologies ..

Today I was going through the changes you made. I plan to have a release soon .. Can u pls enlighten me on some of the following issues:

  1. should I merge this PR ?
  2. what is the current state of the tests ? e.g. I see classes like ClusterUnimplementedMethods, ClusterIncompatibleTests etc. What should we do to these ?
  3. What is the intuition behind the names IntSpec and IntClusterSpec ? (possibly I am missing it)
  4. I see some of the tests e.g. lpush get executed twice when I run testOnly. Guess once through RedisClientSpec and the other time through RedisClusterSpec - right ?
atais commented 4 years ago

Hello. No problem.

Going one by one.

  1. Yes. Cluster was missing some settings available in the client api.
  2. I have achieved my goal with your lib and I did not continue implementing missing methods. Like you have noticed there are two traits: ClusterUnimplementedMethods - This trait should be deleted at some point when all the methods from the related API traits are implemented. ClusterIncompatibleTests - some methods may not be possible to implement in clustered environment and this trait allows you to disable testing that methods.
  3. A really good question... But I am not sure either. I think IntSpec is some naming I inherited from some older class and I named the Cluster accordingly. But it does not make much sense to me now. Just some common class for Specs to inherit from.
  4. Correct. I have pulled all the tests up in hierarchy so that the test behaviour does not rely on the actual implementation. And now you just provide the implementation of either client or cluster and test all the cases.
debasishg commented 4 years ago

@atais Thanks for the response.

Regarding the cluster tests, what we have now is the following ..

describe("cluster operations") {
    shouldSet()
    shouldGetKeysFromProperNodes()
    shouldDoAllOpsOnTheCluster()
    shouldMGet()
    shouldListOps()
    shouldKeyTags()
    shouldReplaceNode()
    shouldRemoveNode()
    shouldListNodes()
  }

But we need to write tests for all other APIs in the cluster e.g. ListApiSpec, HashApiSpec etc. and then we can remove them from ClusterUnimplementedMethods - right ?

atais commented 4 years ago

I think I have covered most of them.

Checking the ClusterUnimplementedMethods only those methods are missing implementation:

  override protected def getConfig(): Unit = ()

  override protected def setConfig(): Unit = ()

  override protected def sort(): Unit = ()

  override protected def sortNStore(): Unit = ()

  override protected def bitop(): Unit = ()

and for example, you can easily find it, like bitop you just go to StringOps for cluster implementation and you will find this method:

override def bitop(op: String, destKey: Any, srcKeys: Any*)(implicit format: Format): Option[Int] = ??? // todo: implement
debasishg commented 4 years ago

Thanks a lot .. I will complete those implementations and hopefully will have a release pretty soon. Thanks for all the contributions.