search

Baqend / Orestes-Bloomfilter

Library of different Bloom filters in Java with optional Redis-backing, counting and many hashing options.
Other
839 stars 245 forks source link

jedis pool #30

Closed So-Far-So-Good closed 7 years ago

So-Far-So-Good commented 8 years ago

you can use JedisSentinelPool instead of JedisPool, What do you think?

ChrisCurtin commented 7 years ago

Do you guys have some direction on changing the RedisPool to use a Pool and provide a way to define which type of Jedis pool to use? I'm thinking about taking this on since we need Sentinel support in our application and wanted to make sure you guys haven't already done it/thought more about it.

Thanks

ChrisCurtin commented 7 years ago

Hi is this project still alive? I tested with sentinel and was able to run as expected with 3 nodes, when forcing the original master to crash the library connected to the new master no problems. Before I refactor all of the tests to support knowing if running in Sentinel, can you give me some direction about my ideas in the WIP pull request?

DivineTraube commented 7 years ago

Not sure, whether my mail reply came through, therefore pasted here again:

Hi Chris,

sorry for the long delay. This project is absolutely still alive, it's a central part of our SaaS and we've just raised a substantial funding round.

We haven't yet started with support for Sentinel and Redis Cluster but will do that in the near future.

should I build a RedisPoolConfiguration class similar to the RedisSentinelConfiguration for consistency?

Yes, I think that is a good idea.

I see a pull request for SSL support. if I add RedisSentinelConfiguration I think it would make sense to add all new pool configuration settings to it, instead of expanding the set of Constructors.

Definitely!

what does the startThread logic do? I couldn't find any tests or where it is used. If needed, can you explain more about why you're creating an explicit Jedis instead of using a resource from the pool?

It is not yet used in the normal Bloom filters. Its purpose is to handle failure-tolerant pubsub connections. You can leave it unchanged.

I've looked at Redis Cluster in Jedis enough to see I can't do the Pool change and have it work with Cluster too. Are you guys working on a refactoring that would work with any connection source?

We haven't started that but we want to add Redis Cluster support soon.

Happy to answer any other questions. Let me know when I should merge your pull requests.

Best, Felix

DivineTraube commented 7 years ago

Hi Chris,

sorry for the long delay. This project is absolutely still alive, it's a central part of our SaaS and we've just raised a substantial funding round.

We haven't yet started with support for Sentinel and Redis Cluster but will do that in the near future.

should I build a RedisPoolConfiguration class similar to the RedisSentinelConfiguration for consistency?

Yes, I think that is a good idea.

I see a pull request for SSL support. if I add RedisSentinelConfiguration I think it would make sense to add all new pool configuration settings to it, instead of expanding the set of Constructors.

Definitely!

what does the startThread logic do? I couldn't find any tests or where it is used. If needed, can you explain more about why you're creating an explicit Jedis instead of using a resource from the pool?

It is not yet used in the normal Bloom filters. Its purpose is to handle failure-tolerant pubsub connections. You can leave it unchanged.

I've looked at Redis Cluster in Jedis enough to see I can't do the Pool change and have it work with Cluster too. Are you guys working on a refactoring that would work with any connection source?

We haven't started that but we want to add Redis Cluster support soon.

Happy to answer any other questions. Let me know when I should merge your pull requests.

Best, Felix

Chris Curtin notifications@github.com schrieb am Di., 18. Okt. 2016 um 03:27 Uhr:

Hi is this project still alive? I tested with sentinel and was able to run as expected with 3 nodes, when forcing the original master to crash the library connected to the new master no problems. Before I refactor all of the tests to support knowing if running in Sentinel, can you give me some direction about my ideas in the WIP pull request?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Baqend/Orestes-Bloomfilter/issues/30#issuecomment-254381413, or mute the thread https://github.com/notifications/unsubscribe-auth/ABgPCipKFcWQU0TrWrodIh3rEzks5Gh9ks5q1CBwgaJpZM4HnTx_ .

ChrisCurtin commented 7 years ago

@DivineTraube I've added my final test cases and support for Sentinel. I also added a class similar to RedisSentinelConfiguration to allow a similar pattern for standalone Redis. I also added some example Redis configuration files and instructions to setup a simple local Redis Sentinel.

Please review and let me know if you have questions. otherwise I think I'm done.

Thanks

Chris

ChrisCurtin commented 7 years ago

Hi, The team working with my changes also added support for multiple databases since our environment requires it. Can you review and accept so we don't have to maintain a separate branch?

Thanks

Chris

fbuecklers commented 7 years ago

We have implemented a test setup for our Jenkins and made some Refactoring of the RedisPool creation. It is merged and released right now.

Regards