Skip to content

Allow Jedis to be selected when both Jedis and Lettuce are on the classpath #22569

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow Jedis to be selected when both Jedis and Lettuce are on the classpath #22569

wants to merge 1 commit into from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Jul 26, 2020

Adds ability to choose Redis client when both Lettuce and Jedis on classpath.

Fixes gh-22559

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 26, 2020
snicoll
snicoll reviewed Jul 27, 2020
View reviewed changes
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've added a suggestion.

...nfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java Outdated
/**
* Client type to use - 'jedis' or 'lettuce'.
*/
private String clientType;
Copy link
Member

Choose a reason for hiding this comment

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

I'd use an enum with Jedis and Lettuce here rather than a raw string. This also improves the developer experience in the IDE.

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 will wait to see what shakes out of #22559 (comment) before updating.

Copy link
Member

@snicoll snicoll Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @Bono007. We've decided to go ahead with the property so if you have time to update your proposal to use an enum that would be much appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good @snicoll. The property approach will be cleaner than the "move the auto-configs to the connection configs" I was proposing as that ended up being a bit messier than I originally thought when I prototyped it.

I will get to it this evening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I squashed and rebased as well.

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue status: on-hold We can't start working on this issue yet and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 27, 2020
@snicoll snicoll mentioned this pull request Aug 4, 2020
Closed
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement and removed status: on-hold We can't start working on this issue yet status: waiting-for-triage An issue we've not yet triaged labels Aug 4, 2020
@snicoll snicoll added this to the 2.4.x milestone Aug 4, 2020
@snicoll snicoll self-assigned this Aug 4, 2020
onobc
onobc commented Aug 5, 2020
View reviewed changes
...nfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java
public enum ClientType {

/**
* Use the Lettuce client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super thrilled about this comment - please update if you have a preference.

@snicoll snicoll changed the title Adds ability to choose Jedis/Lettuce via property Allow Jedis to be selected when both Jedis and Lettuce are on the classpath Aug 5, 2020
snicoll pushed a commit that referenced this pull request Aug 5, 2020
@onobc @snicoll
Add support for selecting the Redis client to use
ac65144 
See gh-22569
snicoll added a commit that referenced this pull request Aug 5, 2020
@snicoll
Polish "Add support for selecting the Redis client to use"
589669d 
See gh-22569
@snicoll snicoll closed this in 0bc5b20 Aug 5, 2020
@snicoll
Copy link
Member

snicoll commented Aug 5, 2020

Thanks @Bono007. I've polished things up in 589669d. In particular, I've removed the default value for clientType as it didn't match the code (the real default is that we configure the client based on the classpath).

@snicoll snicoll modified the milestones: 2.4.x, 2.4.0-M2 Aug 5, 2020
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Aug 5, 2020
@onobc
Copy link
Contributor Author

onobc commented Aug 5, 2020

@snicoll good catch on the default. Thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.4.0-M2
Development

Successfully merging this pull request may close these issues.

Allow Jedis to be selected when both Jedis and Lettuce are on the classpath
3 participants
@onobc @snicoll @spring-projects-issues