Add overloads for XAddOptions in StreamOperations #2936

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

Jump to bottom

Closed

jinkshower wants to merge 1 commit into spring-projects:main from jinkshower:add-overloads-for-xaddoptions-in-streamoperations

Closed

Add overloads for XAddOptions in StreamOperations #2936

jinkshower wants to merge 1 commit into spring-projects:main from jinkshower:add-overloads-for-xaddoptions-in-streamoperations

Conversation

jinkshower

Copy link

Contributor

@jinkshower jinkshower commented Jul 7, 2024 •

edited

Loading

Closes #2915

After this change, the client can use XAddOptions as an additional parameter for opsForStream.add()

TO-BE

XAddOptions options = XAddOptions.maxlen(1).approximateTrimming(false);
redisTemplate.opsForStream.add("myStream", Map.of("key", "value), options);

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).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage label

Jul 7, 2024

jinkshower

jinkshower commented

Jul 7, 2024

View reviewed changes

Copy link

Contributor Author

@jinkshower jinkshower left a comment •

edited

Loading

Choose a reason for hiding this comment

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

please review this pr and if this needs some changes, I will be happy to amend it!

src/main/java/org/springframework/data/redis/connection/ReactiveStreamCommands.java

}

return xAdd(Mono.just(addStreamRecord)).next().map(CommandResponse::getOutput);

}

Copy link

Contributor Author

@jinkshower jinkshower Jul 7, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

the reason why I wrote somewhat verbose method in here is

unlike JedisStreamCommands or LettuceStreamCommands
which both have a method accepting (MapRecord<byte[], byte[], byte[]> record, XAddOptions options)

xAdd(Publisher commands) in LettuceReactiveStreamCommands
expects that Publisher already has variables of XAddOptions.
ex) maxlen, isApproxiateTrimming...

so I placed the decapsulation of XAddOptions here.

I thought of changing AddStreamRecord to have variable priavte final XAddOptions options
so that LettuceReactiveStreamCommands can delegate the decapsulation to other class
but I thought this exceeds the scope of this issue.

jinkshower

jinkshower commented

Jul 7, 2024

View reviewed changes

src/main/java/org/springframework/data/redis/connection/ReactiveStreamCommands.java

* @param commands must not be {@literal null}.

* @return {@link Flux} emitting the {@link RecordId} on by for for the given {@link AddStreamRecord} commands.

* @return {@link Flux} emitting the {@link RecordId} on by for the given {@link AddStreamRecord} commands.

Copy link

Contributor Author

@jinkshower jinkshower Jul 7, 2024

Choose a reason for hiding this comment

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

removed typo

@jinkshower jinkshower changed the title ~~(削除) Add overloads for XAddOptions in StreamOperations. (削除ここまで)~~ (追記) Add overloads for XAddOptions in StreamOperations (追記ここまで)

Jul 7, 2024

@jinkshower jinkshower force-pushed the add-overloads-for-xaddoptions-in-streamoperations branch 2 times, most recently from 5b50789 to f4755e5 Compare

July 10, 2024 11:58

@jinkshower

Copy link

Contributor Author

jinkshower commented Jul 10, 2024

I have a question regarding XAddOptions. While I was writing tests, I found out that

XAddOptions options = XAddOptions.maxlen(-1);
redisTemplate.opsForStream("myStream", Map.of("key", "value), options);

this executes and the result being maxlen option just not applied.

This is becasue in XAddOptions in RedisStreamCommands's hasMaxlen() method.

public boolean hasMaxlen() {
	return maxlen != null && maxlen > 0;
}

also same with AddStreamRecord in ReactiveStreamCommands.
this method just ignores minus value of maxlen.

As for redis, if user makes maxlen -1, An error occurs.
image

If we make hasMaxlen()

public boolean hasMaxlen() {
	return maxlen != null 
}

image
For Lettuce, Lettuce’s XAddArgs has LettuceAssert which asserts the value of maxlen.

image
For Jedis, Jedis’s XAddParams doesn’t have assertions. So redis server throws an exception.

IMO, it is better to alert user by throwing exception than the result just not applied because it is hard to find and also realise the problem.

The reason why I'm asking this question, instead incorporating into PR I created was I was not sure if this is intended.

If this needs to be fixed, I will make adjustment in this PR and push it again. If not, please let me know. Thank you.

@christophstrobl christophstrobl self-assigned this

Jul 15, 2024

@jinkshower jinkshower force-pushed the add-overloads-for-xaddoptions-in-streamoperations branch 2 times, most recently from e667f58 to 3130562 Compare

July 24, 2024 10:22

@jinkshower


 Add overloads for XAddOptions in StreamOperations.

f320d66

Closes spring-projects#2915

@jinkshower jinkshower force-pushed the add-overloads-for-xaddoptions-in-streamoperations branch from 3130562 to f320d66 Compare

July 29, 2024 01:28

@kkocel

Copy link

kkocel commented Jul 31, 2024

@jinkshower looks good to me :)

@jinkshower

Copy link

Contributor Author

jinkshower commented Aug 25, 2024

Hello, @christophstrobl. Could you please take some time to review this PR when you have a moment? I'm not entirely confident about some of the changes I've made, and your feedback would be incredibly valuable to me.

@christophstrobl

Copy link

Member

christophstrobl commented Aug 26, 2024

@jinkshower thanks for the heads up.

christophstrobl pushed a commit that referenced this pull request

Sep 3, 2024

@jinkshower @christophstrobl


 Add overloads for StreamOperations add with additional XAddOptions.

486dc97

Closes: #2915
Original Pull Request: #2936

christophstrobl added a commit that referenced this pull request

Sep 3, 2024

@christophstrobl


 Polishing.

fb0f0bc

Add new method to BoundStreamOperations and remove add accepting a Publisher.
Original Pull Request: #2936

@christophstrobl

Copy link

Member

christophstrobl commented Sep 3, 2024

@jinkshower thank you for your contribution. Merged to main development line.
Regarding the maxlen please open a new issue.

@christophstrobl christophstrobl closed this

Sep 3, 2024

@jinkshower jinkshower mentioned this pull request

Sep 3, 2024

Negative maxlen for XAddOptions is ignored #2982

Closed

Labels

status: waiting-for-triage

4 participants

@jinkshower @kkocel @christophstrobl @spring-projects-issues

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add overloads for XAddOptions in StreamOperations #2936

Add overloads for XAddOptions in StreamOperations #2936

Uh oh!

Conversation

@jinkshower jinkshower commented Jul 7, 2024 •

edited

Loading

Uh oh!

Uh oh!

@jinkshower jinkshower left a comment •

edited

Loading

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jinkshower jinkshower Jul 7, 2024 •

edited

Loading

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jinkshower jinkshower Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

jinkshower commented Jul 10, 2024

Uh oh!

kkocel commented Jul 31, 2024

Uh oh!

jinkshower commented Aug 25, 2024

Uh oh!

christophstrobl commented Aug 26, 2024

Uh oh!

christophstrobl commented Sep 3, 2024

Uh oh!

Uh oh!

Add overloads for XAddOptions in StreamOperations #2936

Add overloads for XAddOptions in StreamOperations #2936

Uh oh!

Conversation

@jinkshower jinkshower commented Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@jinkshower jinkshower left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jinkshower jinkshower Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jinkshower jinkshower Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

jinkshower commented Jul 10, 2024

Uh oh!

kkocel commented Jul 31, 2024

Uh oh!

jinkshower commented Aug 25, 2024

Uh oh!

christophstrobl commented Aug 26, 2024

Uh oh!

christophstrobl commented Sep 3, 2024

Uh oh!

Uh oh!

@jinkshower jinkshower commented Jul 7, 2024 •

edited

Loading

@jinkshower jinkshower left a comment •

edited

Loading

@jinkshower jinkshower Jul 7, 2024 •

edited

Loading