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

feat(stream): add support of the XAUTOCLAIM command #2373

Merged
merged 12 commits into from
Jun 27, 2024

Conversation

LindaSummer
Copy link
Contributor

@LindaSummer LindaSummer commented Jun 20, 2024

Issue

close #2181

Proposed Changes

  • implement XAUTOCLAIM command
  • add test cases following Redis tests

# Comment
1. go Redis client checks count parameter before passing to server, so count == 0 test case can't be created in go test.
2. go Redis client has no delete ids in response, so related test case can't be created.

@PragmaTwice
Copy link
Member

go Redis client checks count parameter before passing to server, so count == 0 test case can't be created in go test.
go Redis client has no delete ids in response, so related test case can't be created.

Maybe we can use redis.Do(ctx, "XAUTOCLAIM", ...) to create test cases.

@aleksraiden aleksraiden requested a review from torwig June 20, 2024 19:33
@LindaSummer
Copy link
Contributor Author

go Redis client checks count parameter before passing to server, so count == 0 test case can't be created in go test.
go Redis client has no delete ids in response, so related test case can't be created.

Maybe we can use redis.Do(ctx, "XAUTOCLAIM", ...) to create test cases.

Thanks for your suggestions. Got it! I will add missing cases and fix CI issues today. 😊

@LindaSummer
Copy link
Contributor Author

LindaSummer commented Jun 21, 2024

Hi @PragmaTwice ,
Thanks for your suggestions.
I have added missing test cases and the missing header algorithm in Darwin build.

Plus, I find that there is warning during Darwin Clang build.
Maybe we should make a new PR to fix it.

/Users/runner/work/kvrocks/kvrocks/src/commands/cmd_stream.cc:145:36: warning: local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]
        if (!result.IsOK()) return result;
                                   ^~~~~~
/Users/runner/work/kvrocks/kvrocks/src/commands/cmd_stream.cc:145:36: note: call 'std::move' explicitly to avoid copying
        if (!result.IsOK()) return result;
                                   ^~~~~~
                                   std::move(result)

@LindaSummer
Copy link
Contributor Author

Hi @PragmaTwice ,

I find that I can run GitHub Actions in my own repo first.
I have run CI in my own repo and fix all build issues now.

Thanks very much.

Best Regards,
Edward

src/commands/cmd_stream.cc Outdated Show resolved Hide resolved
src/commands/cmd_stream.cc Outdated Show resolved Hide resolved
src/commands/cmd_stream.cc Outdated Show resolved Hide resolved
src/types/redis_stream.cc Show resolved Hide resolved
@LindaSummer
Copy link
Contributor Author

Hi @torwig ,

I have updated related code.
Please take a look.
Thanks very much.

Best Regards,
Edward

src/commands/cmd_stream.cc Outdated Show resolved Hide resolved
@git-hulk git-hulk requested a review from torwig June 25, 2024 14:44
src/types/redis_stream.cc Outdated Show resolved Hide resolved
@git-hulk git-hulk requested a review from torwig June 27, 2024 07:05
Copy link

sonarcloud bot commented Jun 27, 2024

@PragmaTwice PragmaTwice merged commit f03d48c into apache:unstable Jun 27, 2024
33 checks passed
@PragmaTwice
Copy link
Member

Thank you for your contribution!

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.

Add support of the XAUTOCLAIM command
3 participants