DATAREDIS-976 - Integrate Cluster-Specific Lettuce PubSub Connection Code #450
Conversation
…cluster by integrating code from the Lettuce wiki at https://github.com/lettuce-io/lettuce-core/wiki/Pub-Sub#redis-cluster
…stener when connection factory is cluster-aware, including the flag to drive additional configuration Lettuce requires to support keyspace event notifications
|
@brucecloud Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@brucecloud Thank you for signing the Contributor License Agreement! |
mp911de
changed the title
|
Hi @brucecloud, thanks for your first contribution. Check out our contribution guidelines that explain the ideal way of contributing to Spring Data projects. Typically, we create a Jira ticket first and then we discuss requirement and solution approaches. I filed DATAREDIS-976 to track this proposal. We'll take a look in the next days. |
|
Thank you for the assistance, @mp911de. |
|
As things stand now, we aren't able to integrate the changes into Moore M4 (Spring Data Redis 2.2 M4). This PR will slip to RC1. |
|
Thank you for the update |
|
I reviewed this PR and this is probably the only way how to integrate Cluster Pub/Sub into Spring Data Redis. Cluster Pub/Sub works similar to Standalone Pub/Sub except for Keyspace notifications. Keyspace events are not propagated across the entire cluster but remain node-local. This introduces a few challenges:
The last point is actually something that worries me because this can easily lead to surprises in mixed setups (listening for keyspace notifications and consuming regular Pub/Sub messages). Beyond that, we also need an implementation for Reactive Pub/Sub. I'm inclined to not include this change as the future of Cluster Pub/Sub is subject to change anyway: https://github.com/antirez/redis/issues/5766#issuecomment-453150075 |
|
For what it is worth we are working on a Spring Session Redis variant that doesn't use events as we have had lots of people complain about the applications being overwhelmed with events. |
|
Reiterating on that PR, we could probably integrate parts of this PR. The bit that causes trouble is enabling keyspace notification listening by subscribing to all masters and listening for events on all nodes. Could you remove the keyspace listening part in preparation for the merge? We can investigate in a second step how you could hook into the subscription to customize it for keyspace listening. |
Hey @mp911de, not sure if I understood this part of your comment correctly, did you refer to Spring Session code when mentioning removal to keyspace listening part? Thing is, with spring-projects/spring-session#1408 we're just adding an alternative Redis based implementation of |
|
@vpavic my comment aimed for this PR and not towards Spring Session. |
|
@mp911de Thanks for the feedback; I apologize, I have been tied up with other work and haven't been able to revisit this. I think my original attempt at what you have suggested was to only enable the keyspace notifications if an 'enableKeySpaceNotifications' flag was set to true when instantiating the LettuceClusterSubscription. If you are suggesting that I should remove that part while the collective mind designs a better implementation for that option, I can certainly do that. Unfortunately it may be a while before I can get back to it though. @rwinch Thanks for all you do with Spring Session, we have been using it for a couple years now. Re: creating a variant of Spring Session just to control/remove the event traffic -- that seems like overkill. If there were just a few more configuration options surfaced it would be easy enough to control which services receive the events, without having to extend RedisHttpSessionConfiguration. Just my two cents. |
All good. No worries. I can take your PR from here, I just didn't want to start without talking with you. The implementation would just enable Cluster Pub/Sub. Ideally, we can transform the code into a form where you are still able to hook into subscriptions (by providing a subclass or a customizer function) to listen to each individual node (with the mentioned drawbacks). |
ClusterConnectionProvider now accepts cluster-specific connection interfaces for Pub/Sub connections. Original pull request: #450.
…n classes. LettuceConnection, LettuceClusterConnection, and LettuceSubscription can now be properly subclassed so they can be extended and created by LettuceConnectionFactory. LettuceConnectionFactory provides template methods doCreateLettuceConnection and doCreateLettuceClusterConnection. Original pull request: #450.
|
After going over the PR, almost nothing was left over. This was still a good exercise as we've identified gaps in tests and we made |
ClusterConnectionProvider now accepts cluster-specific connection interfaces for Pub/Sub connections. Original pull request: #450.
…n classes. LettuceConnection, LettuceClusterConnection, and LettuceSubscription can now be properly subclassed so they can be extended and created by LettuceConnectionFactory. LettuceConnectionFactory provides template methods doCreateLettuceConnection and doCreateLettuceClusterConnection. Original pull request: #450.
|
Superseded by #457. |
…n classes. LettuceConnection, LettuceClusterConnection, and LettuceSubscription can now be properly subclassed so they can be extended and created by LettuceConnectionFactory. LettuceConnectionFactory provides template methods doCreateLettuceConnection and doCreateLettuceClusterConnection. Original pull request: #450 & #457
As part of scaling up our cloud infrastructure, we identified the need to migrate to clustered Redis for our services deployment that uses Spring Session. We are using
Spring Cloud Finchley.SR2
Spring Boot 2.0.5.RELEASE
Spring Session 2.1.4.RELEASE
Spring Data Redis 2.1.5.RELEASE
Lettuce 5.1.4.RELEASE
Testing with the Redis cluster showed that our designated service that processes events from Spring Session was not receiving any session expiration events. Looking closer, Redis keyspace event notification subscriptions required by Spring Session were not being registered on the Redis cluster master nodes. The subscriptions were present in the cluster slave nodes and were indicated in the info for the connections to the slaves, but were missing from the masters.
We looked into the Spring code and the Lettuce documentation and it appears that some of the code Lettuce uses to handle PubSub with clusters hasn't been integrated yet. The documentation in question is here https://github.com/lettuce-io/lettuce-core/wiki/Pub-Sub#redis-cluster.
We successfully integrated the Lettuce code into a branch of Spring Data Redis based on the 2.1.5.RELEASE and Spring Session began working as expected -- our service once again was receiving the session expiration events that are based on the Redis keyspace event notifications, and we can see the PubSub subscriptions on the Redis master nodes.
The Lettuce documentation specifies different code paths for user-space versus keyspace notifications from Redis. For our purposes we hard-coded the modifications to work as required by Spring Session (i.e., for keyspace notifications), but this fork has some additional code to expose the necessary flag to differentiate the two cases. A change will be required in Spring Session to utilize this new flag.
Caveats:
I haven't contributed before so I am looking for feedback/guidance.
This code has been tested as part of our branch based on the 2.1.5.RELEASE. For the fork, I have integrated it into 2.2.0-SNAPSHOT but we haven't tried this upgrade yet.
I am not sure how the unit tests for cluster code work, as I only have a standalone Redis installation in my local dev environment, so I have not attempted to add any unit tests for this functionality.
Thank you