-
Notifications
You must be signed in to change notification settings - Fork 961
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
Introducing JSON to Lettuce #2933
base: redis-json
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! It will be a nice addition to Lettuce.
Just a couple things:
- I did not see tests against Redis Cluster.
- There does not seem to be an implementation of key routing in case of JSON.MGET with the Cluster API.
Hey @jruaux , thanks a lot for taking a look! Yeah these two points are very valid ones. Let me see if I can address them with an update. |
127d29e
to
fc5d14b
Compare
Hey @tishun, |
No, but this is because the PR is fork/redis-json -> origin/redis-json and origin/redis-json is now behind origin/main I will try to fix this. |
….1 (redis#2958) Bumps [org.apache.maven.plugins:maven-failsafe-plugin](https://github.com/apache/maven-surefire) from 3.2.5 to 3.3.1. - [Release notes](https://github.com/apache/maven-surefire/releases) - [Commits](apache/maven-surefire@surefire-3.2.5...surefire-3.3.1) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-failsafe-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
redis#2957) Bumps [org.apache.maven.plugins:maven-javadoc-plugin](https://github.com/apache/maven-javadoc-plugin) from 3.7.0 to 3.8.0. - [Release notes](https://github.com/apache/maven-javadoc-plugin/releases) - [Commits](apache/maven-javadoc-plugin@maven-javadoc-plugin-3.7.0...maven-javadoc-plugin-3.8.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-javadoc-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a good first iteration, but binding all JSON interfaces to generic types of Lettuce codec seems unnecessary and just increases the complexity of the code. Also, we need more developer-friendly overloads for frequently used commands like JSON.SET
. See my inline comments for more details.
src/main/java/io/lettuce/core/api/async/RedisJsonAsyncCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/io/lettuce/core/api/async/RedisJsonAsyncCommands.java
Outdated
Show resolved
Hide resolved
Started JSON README.md; Parser registry is now part of the Connection; formatting; chained all commands; extracted commands in their own unit RedisJsonCommandBuilder;
9b3892b
to
fd842b3
Compare
There is no way to use the Codec system without having the generics, as we discussed offline. I do - however - agree that the
Agreed, this is one of the leftovers |
After some more careful consideration I actually fully agree with you. The latest version does not have any generics for the JsonValue / JsonParser abstractions as they did not help in any way. |
Granted, this review turned out much much much bigger than I anticipated.
Instructions for reviewing
Ideally please concentrate on the most important decisions:
General considerations
(for more details check the README.md with some more information)
This implementation aims to solve several general problems:
Three operating modes facilitating default (basic), advanced and power-user usage
Default mode
Advanced mode
ClientOptions
- to configure a customJsonParser
Power-user mode
Notes
Unrelated changes
There are a unrelated changes such as
Supported commands
Make sure that:
mvn formatter:format
target. Don’t submit any formatting related changes.