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 Redis ClientOptions Customizer #40484

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

Conversation

SOOHYUN-LIM
Copy link

The current LettuceConnectionConfiguration requires too much code to be written when additional settings such as Cluster NodeFilter are needed. Therefore, to enhance scalability, I have added a ClientOptions Customizer.

@pivotal-cla
Copy link

@SOOHYUN-LIM Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 23, 2024
@pivotal-cla
Copy link

@SOOHYUN-LIM Thank you for signing the Contributor License Agreement!

@wilkinsona
Copy link
Member

Thanks for the proposal.

This would leave us with the existing LettuceClientConfigurationBuilderCustomizer that can customize an entire LettuceClientConfigurationBuilder and the new ClientOptionsBuilderCustomizer that can be used to customize the builder that creates the client configuration's ClientOptions. Having a second customizer that targets a subset of another customizer is unusual and I'm not 100% sure what we should do. We could:

  1. Keep things as proposed with some additions to the javadoc of LettuceClientConfigurationBuilderCustomizer to draw attention to ClientOptionsBuilderCustomizer and vice versa
  2. Maybe add a default method to the existing LettuceClientConfigurationBuilderCustomizer for customization of the client options. This may cause problems with the use of method references.
  3. Leave things as they are but this would require duplicating quite a bit of code as, particularly when using SSL, creating the client options is quite involved

1 is probably the best option but I'd like to discuss with the team.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 23, 2024
@philwebb
Copy link
Member

WE discussed this as a team and decided that option 1 is the best way to go. We'd like to rename ClientOptionsBuilderCustomizer to LettuceClientOptionsBuilderCustomizer. We can take care of that when we merge the PR.

@philwebb philwebb added type: enhancement A general enhancement for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Apr 29, 2024
@philwebb philwebb added this to the 3.4.x milestone Apr 29, 2024
@philwebb philwebb removed the for: merge-with-amendments Needs some changes when we merge label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants