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

Add overloads for XAddOptions in StreamOperations #2936

Conversation

jinkshower
Copy link
Contributor

@jinkshower jinkshower commented Jul 7, 2024

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 An issue we've not yet triaged label Jul 7, 2024
Copy link
Contributor Author

@jinkshower jinkshower left a comment

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!

}

return xAdd(Mono.just(addStreamRecord)).next().map(CommandResponse::getOutput);
}
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.

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.

/**
* Add stream record with given {@literal body} to {@literal key}.
*
* @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

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

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

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

@jinkshower thanks for the heads up.

christophstrobl pushed a commit that referenced this pull request Sep 3, 2024
christophstrobl added a commit that referenced this pull request Sep 3, 2024
Add new method to BoundStreamOperations and remove add accepting a Publisher.

Original Pull Request: #2936
@christophstrobl
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add overloads to StreamOperations accepting XAddOptions
4 participants