-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
#998: Added GET.UNWATCH command support and fix watch related issues #1201
Conversation
@JyotinderSingh - Please reivew. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @psrvere! I believe there is a slight misunderstanding on how the command works. I've explained it in my other comment. I don't think it will take too much effort to make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick resolution to the comments @psrvere! The changes look and well tested. Thanks for making the required SDK changes too.
Approved!
Closes #998
Changes
Bug Fixes
handleUnsubscription
function (watch_manager.go
). This prevented execution of the next cleanup block.cmdWatchSubscriptionChan
initialisation that was incorrectly set to nil in PR Changed scope of CmdWatchSubscriptionChan to local #1169