Add support for redis-sentinel in spring.redis.url (Lettuce only) #27373

onobc · 2021-07-17T16:44:18Z

Adds support for setting the following spring.redis.uri when Lettuce is used:

redis-sentnel
rediss-sentinel
redis-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

Get resolution on "Alt Schemes" (above)
Get resolution on "Config Validation" and adjust (or not) accordingly
Update docs

onobc · 2021-07-17T16:45:33Z

...org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationLettuceUrlTests.java

+ *
+ * @author Chris Bono
+ */
+class RedisAutoConfigurationLettuceUrlTests {


I chose to break these out into their own test as the existing test was already pretty unwieldy and adding this in would not help. If this is an issue we can of course consolidate back in.

There isn't a relationship between the main test and the jedis related one. If you want to break things down, why not LettuceConnectionConfigurationTests ?

I guess naming this new one containing the words "AutoConfiguration" and "Lettuce" may make it seem like this new test is the one that tests the Lettuce variant of auto-config when really the main test RedisAutoConfigurationTests does that.

Naming this new test as you recommend is a bit clearer and may produce less confusion. I will adjust.

Ok after going back into the code I think the current name makes sense. Let me summarize my thinking and you can tell me if I am off of what you are thinking about this @snicoll.

Before my changes:

RedisAutoConfiguration:

tests focused on provider agnostic auto-config features (albeit w/ Lettuce being the default provider).

handles the URL configured cases (with default provider)

RedisAutoConfigurationJedisTests:

tests when Jedis is used as the provider

not as exhaustive as the main RedisAutoConfiguration test and only focuses on URL configured cases and other Jedis specifics.

At this point the URL configured cases are handled for both providers from the 2 above tests.

My changes:

Rather than put the new Lettuce specific URL configured cases in RedisAutoConfiguration I split it out into its own Lettuce URL auto-config test and removed any URL configured cases from the main test.

Up to this point the LettuceConnectionConfiguration and JedisConnectionConfiguration are tested implicitly via the RedisAutoConfiguration*Tests. I think naming this Lettuce URL test LettuceConnectionConfigurationTests will leave contributors wondering where the Jedis version is and also it would not be testing all of the LettuceConnectionConfiguration. Therefore I think leaving it named as RedisAutoConfigurationLettuceUrlTests makes sense.

snicoll

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 RedisURI should cover our bases pretty well.

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.

snicoll · 2021-08-16T12:06:55Z

...org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationLettuceUrlTests.java

+ *
+ * @author Chris Bono
+ */
+class RedisAutoConfigurationLettuceUrlTests {


There isn't a relationship between the main test and the jedis related one. If you want to break things down, why not LettuceConnectionConfigurationTests ?

onobc · 2021-08-16T15:06:47Z

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 RedisURI should 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.

wilkinsona · 2021-08-17T08:33:25Z

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.

onobc · 2021-08-21T13:40:08Z

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.

onobc · 2021-08-21T14:33:41Z

@wilkinsona @snicoll
I added the test coverage for the other alt schemes.

onobc · 2021-08-21T15:22:11Z

spring-boot-project/spring-boot-docs/src/docs/asciidoc/data/nosql.adoc

@@ -41,6 +41,41 @@ The following listing shows an example of such a bean:
 include::{docs-java}/features/nosql/redis/connecting/MyBean.java[]
 ----

+You can set the configprop:spring.data.redis.url[] property to change the URL and configure


@Buzzardo would you happen to be available for some of your famously fussy feedback 😺 ?

ℹ️ I based this on the MongoDB URL in the section directly below it.

onobc · 2021-08-21T15:24:47Z

spring-boot-project/spring-boot-docs/src/docs/asciidoc/data/nosql.adoc

+
+The url property supports the `redis` and `rediss` schemes as well as the following additional schemes when using Lettuce:
+
+* `redis+ssl`


The list of schemes we support is only limited by what the Lettuce RedisURI. I was hesitant to link to that detail here though and instead list out the schemes directly.

pirateskipper · 2021-08-22T14:22:38Z

...n/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java

@@ -82,6 +85,11 @@ LettuceConnectionFactory redisConnectionFactory(
 	}

 	private LettuceConnectionFactory createLettuceConnectionFactory(LettuceClientConfiguration clientConfiguration) {
+		if (StringUtils.hasText(this.getProperties().getUrl())) {


if (StringUtils.hasText(getProperties().getUrl())) {

Checkstyle nor format seem to mind but it is not needed and inconsistent w/ the rest of the getProperties invocations in here.

I am not sure about this one. The behavior is that we're checking for specific properties for .cluster or .sentinel and configure that if present. This reverse the trend if an URI is set, only for Lettuce.

This makes it quite hard to resonate about. Also, any of those properties are ignored and fully replaced by the URI, which sounds like quite a big change.

This reverse the trend if an URI is set, only for Lettuce.

True. Now the order they get connection config is:

Jedis: sentinel -> cluster -> standalone (which then uses uri)

Lettuce: url -> sentinel -> cluster -> standalone (which then uses uri)

This does create divergence between the 2 providers. Is the difference between the 2 providers the main concern, or the change in Lettuce connection config behavior?

Also, any of those properties are ignored and fully replaced by the URI, which sounds like quite a big change

Yeh this is why I was initially thinking about adding similar validation to MongoPropertiesClientSettingsBuilderCustomizer - which would force the consumer to use either the url or the direct props.

Is the difference between the 2 providers the main concern, or the change in Lettuce connection config behavior?

Both.

We could adjust the proposal to respect the previous order of sentinel -> cluster -> standalone (which then uses uri) for Lettuce.

@snicoll I went ahead pushed commit that respects the existing order and only deviates when the users specifies a url that has the new schemes. We are guaranteed existing clients do not use any schemes other than "redis" or "rediss" because we enforce that in RedisConnectionConfiguration base class.

This would be a compromise that allows the new schemes (below) to be taken advantage of:

redis-sentinel

rediss-sentinel

redis+ssl

redis+tls

redis-socket

redis+socket

Thoughts?

snicoll

I am not so sure we should be doing this. It's quite impactful and I don't know how we could conciliate the current arrangement while supporting this as an addition. Flagging for team attention again.

snicoll · 2021-09-22T09:43:18Z

...n/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java

@@ -82,6 +85,11 @@ LettuceConnectionFactory redisConnectionFactory(
 	}

 	private LettuceConnectionFactory createLettuceConnectionFactory(LettuceClientConfiguration clientConfiguration) {
+		if (StringUtils.hasText(this.getProperties().getUrl())) {


I am not sure about this one. The behavior is that we're checking for specific properties for .cluster or .sentinel and configure that if present. This reverse the trend if an URI is set, only for Lettuce.

This makes it quite hard to resonate about. Also, any of those properties are ignored and fully replaced by the URI, which sounds like quite a big change.

…hen using Lettuce. Fixes gh-21920

…standard standalone scheme OR redis-sentinel scheme

…ocketConfiguration. Also updated test.

onobc · 2021-10-18T23:00:57Z

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.

onobc commented Jul 17, 2021

View reviewed changes

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 17, 2021

snicoll reviewed Aug 16, 2021

View reviewed changes

onobc commented Aug 21, 2021

View reviewed changes

pirateskipper reviewed Aug 22, 2021

View reviewed changes

snicoll mentioned this pull request Sep 5, 2021

Support redis-socket:// URLs when configuring a Redis client #22273

Closed

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2021

snicoll added this to the 2.6.x milestone Sep 5, 2021

snicoll reviewed Sep 22, 2021

View reviewed changes

snicoll added the for: team-attention An issue we'd like other members of the team to review label Sep 22, 2021

philwebb added the status: on-hold We can't start working on this issue yet label Oct 13, 2021

snicoll modified the milestones: 2.6.x, 2.x Oct 14, 2021

onobc added 5 commits October 18, 2021 17:57

Add support for redis-sentinel and redis-socket in spring.redis.url w…

38d78ba

…hen using Lettuce. Fixes gh-21920

Code review feedback: removed unnecessary "this."

781902e

Add spring.data.redis.url configuration info to nosql.adoc

9b0a941

Only use the new RedisURI approach when the user has specified a non-…

3137566

…standard standalone scheme OR redis-sentinel scheme

Updated LettuceConnectionConfiguration for explicit support of RedisS…

7c17cec

…ocketConfiguration. Also updated test.

onobc added 2 commits October 19, 2021 08:42

Fixed checkstyle and missing quote on asciidoc

a6be564

Wrong path to spring.redis.url in nosql.adoc

9b6fd8d

scottfrederick mentioned this pull request Nov 10, 2021

Support redis-sentinel:// URLs when configuring a Redis client #21920

Closed

philwebb force-pushed the main branch 2 times, most recently from 7aa1646 to 5b11628 Compare November 19, 2021 18:42

philwebb force-pushed the main branch from 1ca278f to 902dd0b Compare November 19, 2021 20:17

snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jan 29, 2022

philwebb modified the milestones: 2.x, 3.x Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for redis-sentinel in spring.redis.url (Lettuce only) #27373

Add support for redis-sentinel in spring.redis.url (Lettuce only) #27373

onobc commented Jul 17, 2021 •

edited

Loading

onobc Jul 17, 2021

snicoll Aug 16, 2021

onobc Aug 16, 2021

onobc Aug 21, 2021

snicoll left a comment

snicoll Aug 16, 2021

onobc commented Aug 16, 2021

wilkinsona commented Aug 17, 2021

onobc commented Aug 21, 2021

onobc commented Aug 21, 2021

onobc Aug 21, 2021

onobc Aug 21, 2021

pirateskipper Aug 22, 2021

onobc Aug 22, 2021

snicoll Sep 22, 2021

onobc Sep 22, 2021

snicoll Oct 1, 2021

onobc Oct 4, 2021

onobc Oct 9, 2021

snicoll left a comment

snicoll Sep 22, 2021

onobc commented Oct 18, 2021


		The url property supports the `redis` and `rediss` schemes as well as the following additional schemes when using Lettuce:

		* `redis+ssl`

Add support for redis-sentinel in spring.redis.url (Lettuce only) #27373

Are you sure you want to change the base?

Add support for redis-sentinel in spring.redis.url (Lettuce only) #27373

Conversation

onobc commented Jul 17, 2021 • edited Loading

Alt Schemes

Config Validation

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before my changes:

My changes:

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onobc commented Aug 16, 2021

wilkinsona commented Aug 17, 2021

onobc commented Aug 21, 2021

onobc commented Aug 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onobc commented Oct 18, 2021

onobc commented Jul 17, 2021 •

edited

Loading