Adds support for setting the following spring.redis.uri when Lettuce is used:
redis-sentnelrediss-sentinelredis-socket
⚠️ This also implicitly adds support for redis-socket:// (#22273)
Alt Schemes
ℹ️ : Because the RedisURI is being used to parse the url string, other url schemes are actually supported - namely:
- redis+ssl
- redis+tls
- redis+socket
Should we block user's from using these alt schemes? It seems that if we don't block, we at least don't advertise the ability to use these alt schemes. @wilkinsona @snicoll wdyt?
Config Validation
I was tempted to add a validateConfiguration` that asserted that if the url was used, only the relevant other props could be used. This would of course, potentially break existing users (both Lettuce and Jedis). Just wanted to get your opinions here as well.
private void validateConfiguration() {
if (!StringUtils.hasText(getProperties().getUrl())) {
return;
}
Assert.state(getProperties().getSentinel() == null || !StringUtils.hasText(getProperties().getSentinel().getMaster()),
"Invalid redis configuration, 'sentinel.master' not supported when 'url' is specified");
Assert.state(getProperties().getSentinel() == null || ObjectUtils.isEmpty(getProperties().getSentinel().getNodes()),
"Invalid redis configuration, 'sentinel.nodes' not supported when 'url' is specified");
Assert.state(this.getProperties().getCluster() == null,
"Invalid redis configuration, 'cluster' properties not supported when 'url' is specified");
}
TODO
- [x] Get resolution on "Alt Schemes" (above)
- [x] Get resolution on "Config Validation" and adjust (or not) accordingly
- [x] Update docs
Comment From: onobc
Thanks for the PR. I don't think the "Alt Schemes" are really a problem if we expand the scope of the PR a bit. Support for
RedisURIshould cover our bases pretty well.
Sounds good. I will probably add some more tests to cover the alt-schemes a bit.
As for the validation, I don't know if failing is really an option. We do something like this in Mongo (
MongoPropertiesClientSettingsBuilderCustomizer) though. I've flagged for team attention to get more feedback.
Sounds good. I will see what you all come back w/ and adjust (or not) accordingly.
Comment From: wilkinsona
As for the validation, I don't know if failing is really an option.
I’m in two minds. On the one hand, given that we don’t have good support for clearing a property’s value, I think it’s safer not to perform the validation. You may have a profile where you’re setting the URL and want the sentinel nodes that are configured elsewhere to be ignored. On the other hand, silently ignoring some configuration may be confusing.
For consistently with (most of) the rest of Boot, I think I’m leaning towards not performing any validation.
Comment From: onobc
As for the validation, I don't know if failing is really an option.
I’m in two minds. On the one hand, given that we don’t have good support for clearing a property’s value, I think it’s safer not to perform the validation. You may have a profile where you’re setting the URL and want the sentinel nodes that are configured elsewhere to be ignored. On the other hand, silently ignoring some configuration may be confusing.
For consistently with (most of) the rest of Boot, I think I’m leaning towards not performing any validation.
Another option would be to log a warning rather than fail. One one hand, this could be helpful to a user when debugging a connection issue. On the other hand, a warn log may not be noticed and that is code we have to add/maintain. I will proceed as-is w/o validation. It can always be added later.
Comment From: onobc
@wilkinsona @snicoll I added the test coverage for the other alt schemes.
Comment From: onobc
Looks like this one got bumped out of 2.6.x. I went ahead and updated w/ the changes I have in order to at least get a clean test run. I am happy to adjust the proposal once the team decides on a direction for the url wrt to Jedis/Redis. Just ping me when that time comes and I will hop back on this.