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

Improve empty stream responses deserialization #1022

Merged
merged 3 commits into from
Aug 3, 2021
Merged

Improve empty stream responses deserialization #1022

merged 3 commits into from
Aug 3, 2021

Conversation

godric
Copy link
Contributor

@godric godric commented Jul 29, 2021

This is to fix an error that still happens despite #905:

When attempting to read trimmed, pending messages for a consumer group from a stream, we still get

NoMethodError: undefined method each_slice' for nil:NilClass

This is because Redis keeps the entry_id despite the entry itself having been deleted, and will respond with, eg:

% redis-cli XREADGROUP GROUP test_group test_consumer STREAMS test_key 0
1) 1) "test_key"
   2) 1) 1) "1627563001384-0"
         2) (nil)

The branch provides a spec to reproduces the issue and validate the fix. Let me know if you need me to do changes.
And thanks for this great gem 👍

PS: This is related to the bug mentioned in issues #936, #929. Although I wasn't able to reproduce the issue with XCLAIM, potentially thanks to the previous fix in #905

test/streams_test.rb Outdated Show resolved Hide resolved
@byroot
Copy link
Collaborator

byroot commented Jul 29, 2021

The CI failures look legit.

@godric godric requested a review from byroot July 30, 2021 13:10
@godric
Copy link
Contributor Author

godric commented Jul 30, 2021

Sorry, I utterly failed my attempt to rebase on redis-rb/master
I also realised one of my assertion was wrong, and fixed it, it should pass the CI now.

@byroot
Copy link
Collaborator

byroot commented Aug 2, 2021

Redis::CommandError: ERR unknown command `xgroup`, with args beginning with: `create`, `k1`, `g1`, `0`, `MKSTREAM`,

You need to handle older redises in the test suite.

class TestStreams < Minitest::Test
include Helper::Client

def test_read_a_trimmed_entry
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go in https://github.com/redis/redis-rb/blob/master/test/lint/streams.rb (that will give you the automatic skip if redis is too old)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the test and moved it with other xreadgroup tests ✅

@godric godric requested a review from byroot August 2, 2021 23:31
@byroot byroot merged commit 97742e0 into redis:master Aug 3, 2021
@byroot
Copy link
Collaborator

byroot commented Aug 3, 2021

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.

2 participants