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 support for redis-sentinel in spring.redis.url (Lettuce only) #27373

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 7 commits into from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Jul 17, 2021

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

*
* @author Chris Bono
*/
class RedisAutoConfigurationLettuceUrlTests {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

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.

*
* @author Chris Bono
*/
class RedisAutoConfigurationLettuceUrlTests {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

onobc commented Aug 16, 2021

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
Copy link
Member

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
Copy link
Contributor Author

onobc commented Aug 21, 2021

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
Copy link
Contributor Author

onobc commented Aug 21, 2021

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

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

* `redis+ssl`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 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 snicoll added this to the 2.6.x milestone Sep 5, 2021
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

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.

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

private LettuceConnectionFactory createLettuceConnectionFactory(LettuceClientConfiguration clientConfiguration) {
if (StringUtils.hasText(this.getProperties().getUrl())) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Sep 22, 2021
@philwebb philwebb added the status: on-hold We can't start working on this issue yet label Oct 13, 2021
@snicoll snicoll modified the milestones: 2.6.x, 2.x Oct 14, 2021
@onobc
Copy link
Contributor Author

onobc commented Oct 18, 2021

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.

@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jan 29, 2022
@philwebb philwebb modified the milestones: 2.x, 3.x Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on-hold We can't start working on this issue yet type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants