Add overloads for XAddOptions in StreamOperations#2936
Add overloads for XAddOptions in StreamOperations#2936jinkshower wants to merge 1 commit intospring-projects:mainfrom
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| } | ||
|
|
||
| return xAdd(Mono.just(addStreamRecord)).next().map(CommandResponse::getOutput); | ||
| } |
There was a problem hiding this comment.
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.
| * | ||
| * @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. |
jinkshower
changed the title
jinkshower
force-pushed
the
add-overloads-for-xaddoptions-in-streamoperations
branch
2 times, most recently
from
5b50789 to
f4755e5
Compare
|
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 public boolean hasMaxlen() {
return maxlen != null && maxlen > 0;
}also same with As for redis, if user makes maxlen -1, An error occurs. If we make hasMaxlen() public boolean hasMaxlen() {
return maxlen != null
}
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. |
jinkshower
force-pushed
the
add-overloads-for-xaddoptions-in-streamoperations
branch
2 times, most recently
from
e667f58 to
3130562
Compare
jinkshower
force-pushed
the
add-overloads-for-xaddoptions-in-streamoperations
branch
from
3130562 to
f320d66
Compare
|
@jinkshower looks good to me :) |
|
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. |
|
@jinkshower thanks for the heads up. |
Add new method to BoundStreamOperations and remove add accepting a Publisher. Original Pull Request: #2936
|
@jinkshower thank you for your contribution. Merged to main development line. |
4 participants
Closes #2915
After this change, the client can use
XAddOptionsas an additional parameter foropsForStream.add()TO-BE