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.

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

Deprecate the STRALGO command and implement the LCS in its place #3037

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

Deprecate the STRALGO command and implement the LCS in its place #3037

wants to merge 2 commits into from

Conversation

Dltmd202
Copy link

Deprecate the STRALGO command and implement the LCS in its place

Is your feature request related to a problem? Please describe
With redis/redis#9744 the STRALGO command was deprecated in Redis 7.0
Tests that test the STRALGO command have not been running since the pipeline only uses latest Redis versions

Describe the solution you'd like
Implement the LCS command

Describe alternatives you've considered
For users of the Lettuce driver the only possible solution now is to use the Custom commands

Closes #2987

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@injae-kim injae-kim mentioned this pull request Nov 3, 2024
Open
@tishun
Copy link
Collaborator

tishun commented Nov 3, 2024

Hey folks, there seem to be test failures, could you check locally if tests pass?

make test

@Dltmd202
Copy link
Author

Dltmd202 commented Nov 3, 2024

I'm really sorry.
I thought all I had to do was mvn test, I would test it, edit the code, and upload it quickly.

@Dltmd202 Dltmd202 force-pushed the topic/Dltmd202/issue-2987 branch 5 times, most recently from 89accb7 to 74ee544 Compare November 3, 2024 18:54
@Dltmd202
Copy link
Author

@tishun I modified the code to make the integration test run successfully!

tishun
tishun requested changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

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

Hey, apologies for the late response, there are a few things that are missing I am afraid.

You can get some ideas from the original PR that introduced STRALGO, but in short:

  • we use a template file to generate the different commands, in this case the src/main/templates/io/lettuce/core/api/RedisStringCommands.java, after adding the new command there you can call the tests in the src/test/java/io/lettuce/apigenerator to generate the different methods in all related classes. You can comment out the other commands in the "Constants" class to avoid generating anything else but the RedisStringCommands.

src/main/java/io/lettuce/core/LcsArgs.java Outdated Show resolved Hide resolved
src/main/java/io/lettuce/core/LcsArgs.java Outdated Show resolved Hide resolved
@Dltmd202
Copy link
Author

Dltmd202 commented Nov 12, 2024 •
edited
Loading

I realized through the template file (src/main/templates/io/lettuce/core/api/RedisStringCommands.java) how it determines the implementation of various connection methods, like reactor and async.

Additionally, I noticed I had missed the NodeSelection series, so I registered that part as well and re-uploaded the code.

Thank you so much for your review @tishun!

@tishun
Copy link
Collaborator

tishun commented Nov 13, 2024

I realized through the template file (src/main/templates/io/lettuce/core/api/RedisStringCommands.java) how it determines the implementation of various connection methods, like reactor and async.

Additionally, I noticed I had missed the NodeSelection series, so I registered that part as well and re-uploaded the code.

Thank you so much for your review @tishun!

If you run the apigenerator test cases they would generate all the required API changes.

Then all you have to do is wire the implementation in.

@tishun tishun added this to the 6.6.0.RELEASE milestone Nov 15, 2024
@tishun
Copy link
Collaborator

tishun commented Nov 15, 2024

Looks awesome, thanks for the contribution!

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

@tishun tishun tishun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.6.0.RELEASE
Development

Successfully merging this pull request may close these issues.

Implement the LCS command and deprecate STRALGO
2 participants
@Dltmd202 @tishun