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

Adds support for SMEMBERS.WATCH command #1289

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

superiorsd10
Copy link

This PR implements the feature described in the issue #1132. The new SMEMBERS.WATCH command allows subscribed clients to receive push responses whenever the data in a respective set is modified.


Changed Introduced

  • Modified the evalSADD function
  • Added command constants
  • Updated command watch manager
  • Defined metadata for SMEMBERS.WATCH
  • Added integration tests

Why did I modified the evalSADD function

I have had to modify this function because the subscribers were receiving double notifications for the first SADD command. The old implementation caused double notifications because it performed a store.PUT for the initial set creation and subsequent additions. The new implementation defers the store.PUT operation until all additions are complete, ensuring subscribers are notified only once.

Why Sorting the Results in Integration Tests Is Necessary

To validate the correctness of the reactive system, we care about the contents and not the order of the set. Sorting both the expected and received results allows us to

  • Ignore the effects of the asynchronous delivery.
  • Focus solely on whether the correct data was transmitted.

Screenshot

SCR-20241116-q7s

Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding support for SMEMBERS.WATCH command @superiorsd10! Had a few small comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you reformat this file? I'm seeing additional diffs here due to indentation changes which makes it harder to see the actual code changes.

})
}

func sortSlice(v any) any {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to use sortSlice? I think we may be able to use testify library's assert.ElementsMatch method

Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes.

@apoorvyadav1111
Copy link
Contributor

Hi @superiorsd10 , Please rebase the PR with latest master, Thanks.

@superiorsd10
Copy link
Author

@apoorvyadav1111 I've rebased it with the latest master. Thank you!

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

Successfully merging this pull request may close these issues.

3 participants