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 spring.data.redis.lettuce.read-from property #42588

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

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Oct 10, 2024

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 10, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Oct 10, 2024

Thanks, @nosan. Unfortunately, when I looked at this yesterday, I mistakenly thought that ReadFrom was an enum. The changes proposed here demonstrate that's not the case. Sorry for any wasted effort here but I'm not sure that I like having three properties (read-from.type, read-from.cidr-notations, and read-from.pattern). A lot of the time, only type will be needed and the other two are only needed when type has a particular value.

I wonder if we could still use a single property. The values that map directly to a ReadFrom constant are straightforward but we'd need some special handling for subnet and regex. Perhaps we could support something like subnet:notationOne,notationTwo,notationThree and regex:pattern? This support could perhaps be implemented as a @ConfigurationPropertiesBinding converter and the property could be typed as a ReadFrom instance.

I don't want to waste any more of your time (apologies again for the time possibly already wasted) so please don't do anything about the above until we've had a chance to discuss this as a team.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Oct 10, 2024
@nosan
Copy link
Contributor Author

nosan commented Oct 10, 2024

Thanks, @wilkinsona.
No problem at all.

@nosan
Copy link
Contributor Author

nosan commented Oct 10, 2024

@Bean
@ConfigurationPropertiesBinding
StringToReadFromConverter stringToReadFromConverter() {
   return new StringToReadFromConverter();
}
	

static class StringToReadFromConverter implements GenericConverter {

		private static final Set<ConvertiblePair> CONVERTIBLE_TYPES;

		static {
			CONVERTIBLE_TYPES = Set.of(new ConvertiblePair(String.class, ReadFrom.class));
		}

		@Override
		public Set<ConvertiblePair> getConvertibleTypes() {
			return CONVERTIBLE_TYPES;
		}

		@Override
		public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
			if (source == null) {
				return null;
			}
			String value = source.toString();
			int index = value.indexOf(':');
			if (index != -1) {
				String type = value.substring(0, index);
				if (type.equalsIgnoreCase("subnet")) {
					return ReadFrom.subnet(StringUtils.commaDelimitedListToStringArray(value.substring(index + 1)));
				}
				if (type.equalsIgnoreCase("regex")) {
					return ReadFrom.regex(Pattern.compile(value.substring(index + 1)));
				}
			}
			return ReadFrom.valueOf(value);
		}

	}
	

in that case, it will be possible to use the following syntax:

spring.data.redis.lettuce.read-from=regex:192.*
spring.data.redis.lettuce.read-from=subnet:192.12.128.0/32,192.168.255.255/32
spring.data.redis.lettuce.read-from=any

@nosan
Copy link
Contributor Author

nosan commented Oct 10, 2024

Is it ok to use ReadFrom field type from io.lettuce.core in RedisProperties.Lettuce properties?

/**
* Defines from which Redis nodes data is read.
*/
private ReadFrom readFrom;

@nosan
Copy link
Contributor Author

nosan commented Oct 10, 2024

perhaps it would be beneficial to reach out to someone from the Lettuce team to explore the possibility of adding support for the regex and subnet types in the ReadFrom.valueOf(...) method

At the moment:

 //ReadFrom valueOf(String name)
 
        if (name.equalsIgnoreCase("subnet")) {
            throw new IllegalArgumentException("subnet must be created via ReadFrom#subnet");
        }

        if (name.equalsIgnoreCase("regex")) {
            throw new IllegalArgumentException("regex must be created via ReadFrom#regex");
        }
        

@wilkinsona
Copy link
Member

perhaps it would be beneficial to reach out to someone from the Lettuce team

I think that's a good idea. WDYT, @mp911de, about supporting subnet:… and regex:… or similar syntax with ReadFrom.valueOf to ease property-based configuration?

@wilkinsona wilkinsona added status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Oct 14, 2024
@mp911de
Copy link
Member

mp911de commented Oct 15, 2024

I like the idea a lot, especially flexibility introduced with regex: and subnet: variants is pretty exciting.

@wilkinsona
Copy link
Member

Thanks, @mp911de. I've opened redis/lettuce#3013 for further consideration.

@wilkinsona wilkinsona removed status: waiting-for-triage An issue we've not yet triaged status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Oct 18, 2024
@wilkinsona wilkinsona added this to the 3.x milestone Oct 18, 2024
@wilkinsona wilkinsona added type: enhancement A general enhancement status: blocked An issue that's blocked on an external project change labels Oct 18, 2024
@nosan nosan force-pushed the gh-42576 branch 3 times, most recently from 0349f8f to 2ae3070 Compare October 18, 2024 19:20
@nosan nosan changed the title Add spring.redis.lettuce.read-from property Add spring.data.redis.lettuce.read-from property Oct 18, 2024
@nosan
Copy link
Contributor Author

nosan commented Oct 18, 2024

@wilkinsona
redis/lettuce#3016 It's been merged and will be available in Lettuce 6.5.0.RELEASE.
Meanwhile, I've updated the PR. Could I kindly ask you to review it once again, please?

I have the following questions:

  • How can I correctly map kebab-case values? ReadFrom.valueOf supports case-insensitive names (e.g., upstreampreferred). I used readFrom.replaceAll("-", "")
  • What values should I use for regex and subnet in additional-spring-configuration-metadata.json?

@philwebb
Copy link
Member

How can I correctly map kebab-case values? ReadFrom.valueOf supports case-insensitive names (e.g., upstreampreferred). I used readFrom.replaceAll("-", "")

I think we can use the same logic as in LenientObjectToEnumConverterFactory. I've done that in bea7704

What values should I use for regex and subnet in additional-spring-configuration-metadata.json?

I think the ones you've added are correct.

@nosan
Copy link
Contributor Author

nosan commented Oct 18, 2024

Thank you, @philwebb
I cherry-pick your commit 👍

@wilkinsona
Copy link
Member

I wonder if we should hold off on documenting regex: and subnet: in the metadata until we're using Lettuce 6.5 by default. The feature's still useful before then and if someone using Lettuce 6.4 wants to configure a regex or subnet read from, they can always use a customizer till Lettuce 6.5 is out and they've overridden the version or we've upgraded.

@nosan
Copy link
Contributor Author

nosan commented Oct 21, 2024

I wonder if we should hold off on documenting regex: and subnet: in the metadata until we're using Lettuce 6.5 by default. The feature's still useful before then and if someone using Lettuce 6.4 wants to configure a regex or subnet read from, they can always use a customizer till Lettuce 6.5 is out and they've overridden the version or we've upgraded.

It depends on when it gets merged :) If you think it's ready to be merged, I can remove subnet: and regex: from the metadata.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Oct 21, 2024
@nosan
Copy link
Contributor Author

nosan commented Oct 21, 2024

Another approach could be to temporarily add support for regex: and subnet: in Spring Boot, and then remove it once version 6.5.0 is released.

@wilkinsona
Copy link
Member

We discussed this today and decided that we're going to err on the side of caution and wait till Lettuce 6.5 is out before merging anything. Hopefully that'll be in the Boot 3.5 timeframe.

@nosan
Copy link
Contributor Author

nosan commented Oct 21, 2024

Thank you for your feedback, @wilkinsona.
Feel free to reach out if any further work is required on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants