Skip to content

Add NOMKSTREAM option to XADD command #2047

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

Closed
4 tasks done
mp911de opened this issue Apr 19, 2021 · 14 comments
Closed
4 tasks done

Add NOMKSTREAM option to XADD command #2047

mp911de opened this issue Apr 19, 2021 · 14 comments
Assignees
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors theme: Redis 6.2 type: enhancement A general enhancement

Comments

@mp911de
Copy link
Member

mp911de commented Apr 19, 2021

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have contributed before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Problem

Redis 6.2 introduced a new flag, NOMKSTREAM to its XADD command. We want to support this flag in Spring Data Redis.

Solution

We have XAddOptions as a collector for additional arguments for the XADD command. Extend XAddOptions (for the imperative API) and AddStreamRecord (for the reactive API). Add tests to verify this functionality.

See also https://redis.io/commands/xadd for further information.

Steps to Fix

  • Claim this issue with a comment below and ask any clarifying questions you need
  • Set up a repository locally following the Contributing Guidelines
  • Try to fix the issue following the steps above
  • Commit your changes and start a pull request
@mp911de mp911de added type: enhancement A general enhancement theme: Redis 6.2 status: first-timers-only An issue that can only be worked on by brand new contributors labels Apr 19, 2021
@ashwanirai07
Copy link

ashwanirai07 commented Apr 24, 2021

Hello @mp911de, I am new to open source contribution and wanted to contribute to this issue. So can you pls assign me the issue? Pls, specify the exact changes that need to be done(the class files, methods ) that need to be changed.

@mp911de
Copy link
Member Author

mp911de commented Apr 24, 2021

Thanks for asking. Check out the description above. It contains all production code classes.

@mp911de
Copy link
Member Author

mp911de commented Jun 11, 2021

Clearing assignee because of inactivity.

@omlip
Copy link

omlip commented Jun 11, 2021

@mp911de may I help ?

@mp911de
Copy link
Member Author

mp911de commented Jun 11, 2021

Sure. I assigned the ticket to you. Let me know if there's something we can help you with.

@omlip
Copy link

omlip commented Jun 11, 2021

@mp911de I am sorry but I just notice that it was a first timer only issue and not an ideal for contributions but I already participated in such issue with spring-boot project so it is not really fair if I take it in order to respect the first timers only concept. Are you agree with me ?

@mp911de
Copy link
Member Author

mp911de commented Jun 11, 2021

No worries. We've announced this issue for the second time after quite some time of inactivity. Since you're new to Spring Data, technically you're a first-timer here.

@mp911de
Copy link
Member Author

mp911de commented Jun 24, 2021

Any update here? Is there something we can help you with @omlip?

@omlip
Copy link

omlip commented Jun 25, 2021

@mp911de You did well to remind me.
In fact the goal of this task is "just" to extend the 2 classes and backed fields to supprot this flag or this task is going further and add a deeper support for nomkstream flag ?

@mp911de
Copy link
Member Author

mp911de commented Jul 1, 2021

It is to extend both classes and adding tests for these.

@mp911de
Copy link
Member Author

mp911de commented Jul 12, 2021

Clearing assignee because of inactivity.

@morenomjc
Copy link
Contributor

Hi @mp911de can I take this on?

@mp911de
Copy link
Member Author

mp911de commented Jul 12, 2021

Sure, I assigned the ticket to you. Let us know if you have any questions.

morenomjc pushed a commit to morenomjc/spring-data-redis that referenced this issue Jul 13, 2021
@morenomjc
Copy link
Contributor

Hi @mp911de submitted an initial PR. I based the unit tests from similar changes.

@mp911de mp911de added this to the 2.6 M1 (2021.1.0) milestone Jul 13, 2021
morenomjc added a commit to morenomjc/spring-data-redis that referenced this issue Jul 13, 2021
mp911de added a commit that referenced this issue Jul 14, 2021
Switch Jedis XADD call to XAddParams.

See #2047
Original pull request: #2118.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors theme: Redis 6.2 type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants