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

Java: `JSON.MGET`. #2514

Merged

Yury-Fridlyand merged 6 commits into release-1.2 from java/yuryf-json.mget-valkey-447

Nov 14, 2024

Collaborator

Yury-Fridlyand commented Oct 24, 2024 •

edited

Loading

Issue link

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Commits will be squashed upon merging.


          JSON.MGET.

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand added the java label

Yury-Fridlyand requested a review from a team as a code owner

October 24, 2024 20:56

Yury-Fridlyand temporarily deployed to AWS_ACTIONS

October 24, 2024 20:56

— with

GitHub Actions Inactive

Yury-Fridlyand temporarily deployed to AWS_ACTIONS

October 24, 2024 20:56

— with

GitHub Actions Inactive

Yury-Fridlyand had a problem deploying to AWS_ACTIONS

October 24, 2024 20:56

— with

GitHub Actions Failure

Yury-Fridlyand changed the title ~~JSON.MGET.~~ Java: JSON.MGET.

Yury-Fridlyand commented

View reviewed changes

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved

Yury-Fridlyand mentioned this pull request

Java: Json support #2430

Closed

22 tasks

Yury-Fridlyand linked an issue

that may be closed by this pull request

Allow sending keys from different slots on JSON.MGET for CME #2455

Closed

Yury-Fridlyand mentioned this pull request

Allow sending keys from different slots on JSON.MGET for CME #2455

Closed

Yury-Fridlyand requested a review from shohamazon

October 25, 2024 17:10

shohamazon requested a review from barshaul

October 27, 2024 08:30

barshaul previously requested changes

View reviewed changes

Collaborator

barshaul left a comment

see comments inline

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved

Yury-Fridlyand dismissed barshaul’s stale review

November 7, 2024 21:17

Dropping these comments, becase GLIDE core changes were done in #2587

Yury-Fridlyand added 2 commits

November 7, 2024 13:25


          update

f380a34

Signed-off-by: Yury-Fridlyand <[email protected]>


          Merge remote-tracking branch 'upstream/release-1.2' into java/yuryf-j…

288ef6c

…son.mget-valkey-447

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand had a problem deploying to AWS_ACTIONS

November 7, 2024 21:28

— with

GitHub Actions Error


          update

ea14d71

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS

November 7, 2024 21:29

— with

GitHub Actions Inactive

Collaborator Author

Yury-Fridlyand commented Nov 7, 2024

jamesx-improving reviewed

View reviewed changes

java/client/src/main/java/glide/api/commands/servermodules/Json.java

+                          @NonNull BaseClient client, @NonNull String[] keys, @NonNull String path) {
+                      return Json.<Object[]>executeCommand(
+                                      client, concatenateArrays(new String[] {JSON_MGET}, keys, new String[] {path}))
+                              .thenApply(res -> castArray(res, String.class));

Collaborator

jamesx-improving Nov 13, 2024

A bit of confusion here. So 3 things:

the thenApply() call here on line 452
the <Object[]> type hint before invoking executeCommand() on line 450
the final convertion in executeCommand(), which will convert to whatever the method return type is (.thenApply(r -> (T) r)) on line 2709 (for mget(), T is String[])

Not sure why all 3 are needed here. Isn't 3 alone good enough?

Collaborator Author

Yury-Fridlyand Nov 13, 2024

lib.rs returns an arbitrary Object[], it cannot be casted to String[], so we have to use castArray.
So what happens here:

libglide returns Object[]
customCommand returns Object (which is actually array, yes)
with Json.<Object[]> prefix we instruct that we expect Object[]
with .thenApply(r -> (T) r) we safely cast this Object to Object[]
with castArray we convert Object[] to String[]

So in total there is a chained Object -> Object[] -> String[] conversion distrubuted over multiple places of code 🤷
I'd like to refactor this if you give me an idea.

Collaborator

acarbonetto Nov 13, 2024

Also note: we kind of have to go through Object -> Object[] -> String[].
This is because castArray requires the Object[] type. We cannot cast Object[] to String[] directly, but we need to cast all inner elements (thank you Java).

java/client/src/main/java/glide/api/commands/servermodules/Json.java Outdated Show resolved Hide resolved


          Update java/client/src/main/java/glide/api/commands/servermodules/Jso…

efc3934

…n.java

Co-authored-by: James Xin <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS

November 13, 2024 00:48

— with

GitHub Actions Inactive

acarbonetto approved these changes

View reviewed changes

java/client/src/main/java/glide/api/commands/servermodules/Json.java Outdated Show resolved Hide resolved

java/client/src/main/java/glide/api/commands/servermodules/Json.java

+                          @NonNull BaseClient client, @NonNull String[] keys, @NonNull String path) {
+                      return Json.<Object[]>executeCommand(
+                                      client, concatenateArrays(new String[] {JSON_MGET}, keys, new String[] {path}))
+                              .thenApply(res -> castArray(res, String.class));

Collaborator

acarbonetto Nov 13, 2024

Also note: we kind of have to go through Object -> Object[] -> String[].
This is because castArray requires the Object[] type. We cannot cast Object[] to String[] directly, but we need to cast all inner elements (thank you Java).

acarbonetto reviewed

View reviewed changes

java/client/src/main/java/glide/api/commands/servermodules/Json.java Show resolved Hide resolved


          Update java/client/src/main/java/glide/api/commands/servermodules/Jso…

a196606

…n.java

Co-authored-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS

November 14, 2024 00:47

— with

GitHub Actions Inactive

jonathanl-bq approved these changes

View reviewed changes

Yury-Fridlyand merged commit d052c4c into release-1.2

15 checks passed

Yury-Fridlyand deleted the java/yuryf-json.mget-valkey-447 branch

November 14, 2024 01:02

prateek-kumar-improving pushed a commit that referenced this pull request


          Java: JSON.MGET. (#2514)

e4cea87

* `JSON.MGET`.

Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: James Xin <[email protected]>
Co-authored-by: Andrew Carbonetto <[email protected]>

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

Reviewers

jamesx-improving jamesx-improving left review comments

acarbonetto acarbonetto approved these changes

jonathanl-bq jonathanl-bq approved these changes

barshaul barshaul left review comments

shohamazon Awaiting requested review from shohamazon

Labels

java