bug(streams): fixing xreadgroup's behavior to be compatible with Redis

[theyueli]

fixes #1860

Therefore, now:

127.0.0.1:6379> flushdb
OK
127.0.0.1:6379> "XREADGROUP" "GROUP" group consumer COUNT 10 BLOCK 5000 STREAMS stream >
(error) NOGROUP No such key 'stream' or consumer group 'group' in XREADGROUP with GROUP option

(used to block for 5 seconds..)

and

127.0.0.1:6379> flushdb
OK
127.0.0.1:6379> "XGROUP" "CREATE" stream group "0" MKSTREAM
OK
127.0.0.1:6379> keys *
1) "stream"
127.0.0.1:6379> "XREADGROUP" "GROUP" group consumer COUNT 10 BLOCK 5000 STREAMS stream >
(nil)
(5.02s)

(used to return immediately without blocking)

[theyueli]

hi @Abhra303 @andydunstall , i saw you implemented the code that I modified... could you please help check if that makes sense? Thanks!

[Abhra303]

Actually, I didn't want to send an error here because, in case of multiple keys given, we may want to receive entries from existing groups (excluding the non-existent group). Wdyt?

[theyueli]

if we do not send error here, I'm afraid that may lead to different behavior than Redis (as it returns error right away if any key or group doesn't exist)? did I miss anything?

[theyueli]

i think you are right.. i just verified this behavior with Redis, will make a change.

[theyueli]

@Abhra303 after trying more example with Redis, I found Redis also returns no group error as long as there is one group that can't be found for xreadgroup. I tested on Redis with the following unit test (modified from your previous test):

  resp = Run({"xreadgroup", "group", "group", "alice", "streams", "mystream", "foo", ">", ">"});
  // returns no group error as "group" was not created for mystream.
  EXPECT_THAT(resp, ArgType(RespExpr::ERROR));

[romange]

@theyueli, Abrha is mostly working on control plane stuff. Therefore, I prefer that Dragonfly's PRs will be approved by core-team members. @kostasrim , @chakaz , @adiholden are good candidates depending on day of week and holidays :)

You can still consult with Abrhadeep about the changes, of course.

[theyueli]

@theyueli, Abrha is mostly working on control plane stuff. Therefore, I prefer that Dragonfly's PRs will be approved by core-team members. @kostasrim , @chakaz , @adiholden are good candidates depending on day of week and holidays :)

You can still consult with Abrhadeep about the changes, of course.

sounds good! just tagged the core team members as reviewers.

[kostasrim]

Making a general comment here (no need for action on your side on this PR).

I really do not like that we conclude transactions manually, it should be covered in some RAII class such that OnScopeExit it concludes itself. Maybe a good solution for this pattern is the Abseil variant called Cleanup. And yes I know we conclude before we Send and error but honestly, it won't affect the performance if we literally conclude after we report the error...

[theyueli]

I really do not like that we conclude transactions manually, it should be covered in some RAII class such that OnScopeExit it concludes itself.

I was asking @romange about doing RAII for this just a moment... I found out I need to conclude transaction so that unit test won't crash..

[Abhra303]

LGTM