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.

Sign up for GitHub

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

Jump to bottom

Add idle attribute for xpending command #2101

Closed
wants to merge 2 commits into from
Closed

Add idle attribute for xpending command #2101

wants to merge 2 commits into from

Conversation

SivaTharun
Copy link

@SivaTharun SivaTharun commented Jun 28, 2021 •
edited
Loading

Overview

idle attribute support for redis xpending stream commands
Connects #2046

  • added the support for idle attribute support for xpending commands in reactive stream commands and redis stream commands
  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@SivaTharun
2046 - implementation the idle attribute support for xpending command
230ab7a 
- added the support for idle attribute support for xpending commands in reactive stream commands and redis stream commands
@pivotal-cla
Copy link

@SivaTharun Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 28, 2021
mp911de
mp911de requested changes Jun 28, 2021
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I had a look and the code looks pretty decent. It would make sense to add integration tests (see AbstractConnectionIntegrationTests and LettuceReactiveStreamCommandsIntegrationTests).

src/main/java/org/springframework/data/redis/core/StreamOperations.java
* @see <a href="https://redis.io/commands/xpending">Redis Documentation: xpending</a>
* @since 2.3
*/
PendingMessages pending(K key, String group, Range<?> range, long count, long idleMilliSeconds);
Copy link
Member

Choose a reason for hiding this comment

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

For both added methods (pending), it would make sense to accept idleTime as long and have a TimeUnit to avoid potential conversion bugs in calling code.

src/main/java/org/springframework/data/redis/connection/StringRedisConnection.java
* @return pending messages for the given {@literal consumer group} or {@literal null} when used in pipeline /
* transaction.
* @see <a href="https://redis.io/commands/xpending">Redis Documentation: xpending</a>
* @since 2.3
Copy link
Member

Choose a reason for hiding this comment

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

@since should point to version 2.6 in all newly introduced methods.

@mp911de mp911de changed the title 2046 - implementation the idle attribute support for xpending command Add idle attribute for xpending command Jul 12, 2021
@christophstrobl
Copy link
Member

@SivaTharun any update on the comments @mp911de had?
Also, please sign the CLA as we cannot consider your contribution without it.

@christophstrobl christophstrobl added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 14, 2021
@SivaTharun SivaTharun marked this pull request as ready for review July 15, 2021 05:23
@SivaTharun
Copy link
Author

SivaTharun commented Jul 15, 2021 •
edited
Loading

@SivaTharun any update on the comments @mp911de had?
Also, please sign the CLA as we cannot consider your contribution without it.

@christophstrobl i am currently addressing the comments, and will be ready in a couple of days. sure i will sign in the CLA

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 15, 2021
@SivaTharun
Merge branch 'main' of github.com:spring-projects/spring-data-redis i…
f1a676f 
…nto issue/2046

# Conflicts:
#	src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java
@mp911de
Copy link
Member

mp911de commented Mar 10, 2022

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@mp911de mp911de closed this Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de requested changes

Assignees

@christophstrobl christophstrobl

Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SivaTharun @pivotal-cla @christophstrobl @mp911de @spring-projects-issues