Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for cluster keyspace notifications to subscriber #1536

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ercangorgulu
Copy link

Adding support for cluster keyspace notifications to subscriber by allowing subscription to multiple nodes

Refactored code for previous pull request based on new changes
#813

Fixes: #789

@shaunco
Copy link

shaunco commented Jun 8, 2022

@NickCraver - Any chance we can get this merged? Getting keyspace notification support would be amazing.

@NickCraver
Copy link
Collaborator

@shaunco Unfortunately not - we've been heads down on some other bits here but the reason this wasn't taken originally was pub/sub was screwed in ways we didn't understand and took a decent deep dive to figure out recently. As a result, the entirely of pub/sub code has been rewritten to behave and hash correctly, fundamentally breaking this PR as well (but fixing all existing use cases).

We're not against adding this feature, but it has a critical component of subscribing this specific thing to all servers which isn't how the rest of subscriptions work. We'd need to handle this case including how to subscribe and re-subscribe since it behaves differently at every level (rather than opportunistically subscribing to any viable server when problems occur like other subscriptions).

Now that we have subscriptions as a base stable and working, this is much more viable to approach, but no time allocation from Marc and I at the moment.

@ercangorgulu We really do appreciate the effort here - hopefully the above makes sense on why changing an unstable system is not desirable (and I should have written this a long time ago). Now that we have the base stable, this is a problem we could go after.

@ercangorgulu
Copy link
Author

@NickCraver thank you for your feedback.
I know this feature is so niche, but we needed to use in our app. Since I already did this for us, I wanted to contribute too.
But after a year or so we also stopped using that.
Previously cache was stored in memory and redis. So we needed to update those automatically. For some parts we wrote additional layer so it handles this with regular pubsub and for other parts we are just storing required caches in request scope.
So basically we removed our dependency of this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cluster pub/sub: keyspace notification issues
3 participants