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

fix(stream): change XREADGROUP response for empty read response #2569

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

jonathanc-n
Copy link
Contributor

@jonathanc-n jonathanc-n commented Oct 2, 2024

references #2568

Changed XReadGroup response for empty responses to match redis behaviour.

@torwig
Copy link
Contributor

torwig commented Oct 2, 2024

@jonathanc-n Should we change existing tests or add extra tests for this case if they don't exist?

@jonathanc-n
Copy link
Contributor Author

jonathanc-n commented Oct 2, 2024

@torwig changes in this particular pr are negligible so probably doesn't need a test, but for the other half of the issue I believe there should be a test.

@torwig
Copy link
Contributor

torwig commented Oct 2, 2024

@jonathanc-n How to be sure that bug was fixed? Exactly, write a test :)

There is a classic flow for fixing a bug:

  1. Write a test that fails.
  2. Fix a bug.
  3. Assert that the test passes -> it will prevent any regression.

@jonathanc-n
Copy link
Contributor Author

I see, thank you for letting me know! :)

@jonathanc-n
Copy link
Contributor Author

@torwig The test is added

torwig
torwig previously approved these changes Oct 2, 2024
@jonathanc-n
Copy link
Contributor Author

jonathanc-n commented Oct 2, 2024

@torwig I made some changes to the stream, hopefully it passes all the checks now

@PragmaTwice PragmaTwice changed the title fix(stream): Change XReadGroup Response For Empty Read Response fix(stream): Change XReadGroup response for empty read response Oct 2, 2024
@PragmaTwice PragmaTwice changed the title fix(stream): Change XReadGroup response for empty read response fix(stream): change XReadGroup response for empty read response Oct 2, 2024
@PragmaTwice PragmaTwice changed the title fix(stream): change XReadGroup response for empty read response fix(stream): change XREADGROUP response for empty read response Oct 2, 2024
@jonathanc-n
Copy link
Contributor Author

@PragmaTwice Do you know why this might be failing? I am not too sure myself

@PragmaTwice
Copy link
Member

PragmaTwice commented Oct 3, 2024

It keeps failing even after multiple retries.

I'll look into it soon.

Copy link

sonarcloud bot commented Oct 3, 2024

@torwig torwig merged commit 149d4fb into apache:unstable Oct 3, 2024
32 checks passed
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