search

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

Common interface for single redis client and cluster classes #243

Closed atais closed 5 years ago

atais commented 5 years ago

Preparing to fix what I have mentioned in #242

any method from RedisCommand that is not overridden will not work. Simply because in RedisShards single host variables are dummies

Changelist:

In the next step, I would like to add some missing methods that I need for my project.

This PR does not affect any logic.

Important change for future: In future, all new methods should be added to API package which will force the user to provide an implementation for all classes.

atais commented 5 years ago

I think those tests fail randomly!? :scream:

debasishg commented 5 years ago

@atais I will take a look over the weekend. Apologies - am too busy with work right now .. Thanks for the PR ..

atais commented 5 years ago

I tried multiple times and the tests are indeed failing randomly. Look at commit status at my branch: https://github.com/atais/scala-redis/commits/api-cleanup

All green.

debasishg commented 5 years ago

The api layer that u introduced looks nice. I was thinking of something similar since some time now but could not get around to work on it for other engagements.

I have started looking into this - will take a couple of more days to go through the entire stuff. Just a couple of observations -

  1. will u pls. complete some of the methods that u have tagged as to-do with ??? ?
  2. I merged a couple of older PRs and now we have a minor merge conflict - could u pls resolve it ?

Please ping me when u r done - I think it can be merged. Just wanted to ensure that all public APIs remain the same and no user code will break.

atais commented 5 years ago

Hello @debasishg

I merged a couple of older PRs and now we have a minor merge conflict - could u pls resolve it ?

Done :)

will u pls. complete some of the methods that u have tagged as to-do with ??? ?

This one is more complicated.

As I mentioned before, the number of unimplemented methods remains the same. Just now, since they do not have the default implementation, the problem is more visible. Anyway, I will need to add some methods because I need them for my project :smile:, and will do so in this or next PR in upcoming days with tests and everything. Probably though, I will not implement all of them.

Just wanted to ensure that all public APIs remain the same and no user code will break.

The API remains 100% the same, I copied all the methods from client interfaces and removed the implementation. The only thing I changed is that I did not copy some @deprecated methods into the API. So the cluster will not have them.

atais commented 5 years ago

Damn, this part of the library with RedisCluster and RedisShards was really dusty.

So... I have tried generalizing everything, having some common interface and tests. Now I will move to implement and test that API in the proper way.

debasishg commented 5 years ago

Indeed this part was added for some specific use case and didn’t get much love. Meanwhile redis cluster came into being and I think users stopped using this. And to be honest I don’t get any time at all to look after this project. Thanks for working on this ..

On Fri, 16 Aug 2019 at 8:19 PM, Michał Siatkowski notifications@github.com wrote:

Damn, this part of the library with RedisCluster and RedisShards was really dusty.

So... I have tried generalizing everything, having some common interface and tests. Now I will move to implement and test that API in the proper way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/debasishg/scala-redis/pull/243?email_source=notifications&email_token=AAA2FX2IOKJ7S5XSAR2LVLTQE246NA5CNFSM4IKBNYJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4O2HEQ#issuecomment-522036114, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2FXZ3MDQSW7AI4C4QPHDQE246NANCNFSM4IKBNYJA .

-- Sent from my iPhone

atais commented 5 years ago

Btw do you know what could be potentially the problem of tests failing? This time it looks like it hanged for no reason.

I could try to address this issue as well

debasishg commented 5 years ago

I am not sure .. that was the reason I removed Travis form this project. It got back only recently.

debasishg commented 5 years ago

build for scala 2.13.0 ran for jdk8 but failed for jdk11 - rerunning ..

atais commented 5 years ago

166 as well