Split JedisClusterCommand into multiple methods #2355

walles · 2021-01-25T08:29:51Z

No behavior changes, just a refactoring.

Changes:

Replaces recursion with a for loop
Extract redirection handling into its own method
Extract connection-failed handling into its own method

I did try to keep the order of the new methods the same order as the previous catch blocks.

Note that tryWithRandomNode is gone, it was never true so it and its code didn't survive the refactoring.

I think redirection handling looks a bit strange, but it didn't change from before. If it is wrong then that should be fixed in another PR, this one is just a refactoring.

Reviewer notes

The git diff isn't great for this change.

If you want to see what changed it's probably easier to bring the old and the new code up side by side and compare that way instead.

Here's the old version for reference: https://github.com/redis/jedis/blob/master/src/main/java/redis/clients/jedis/JedisClusterCommand.java

sazzad16

This is more of a code suggestion. We'll almost always need a normal connection getter. Why not have one final defaultConnectionGetter created within constructor and use it everywhere else?

src/main/java/redis/clients/jedis/JedisClusterCommand.java

Inspired by redis#1334 where this went real easy :). Would have made redis#2355 shorter. Free public updates for JDK 7 ended in 2015: <https://en.wikipedia.org/wiki/Java_version_history> For JDK 8, free public support is available from non-Orace vendors until at least 2026 according to the same table. And JDK 8 is what Jedis is being tested on anyway: <https://github.com/redis/jedis/blob/ac0969315655180c09b8139c16bded09c068d498/.circleci/config.yml#L67-L74>

walles · 2021-01-26T07:26:29Z

AFAWCT we have no outstanding tasks here and are awaiting review.

So for clarity, do you think this is ready for review / merge @sazzad16, or are you expecting any more changes from our side?

sazzad16 · 2021-01-26T08:20:13Z

@walles According to community rule, we need approval of at least two collaborators. Let me see what I can do.
In the mean time, you can wait or continue to work on another PR based on this PR.

walles · 2021-02-01T13:02:16Z

@sazzad16 is there anything we can do to get this reviewed?

Given that me and @jensgreen pair programmed this and you reviewed it, we are three people who approve of these changes so far.

sazzad16 · 2021-02-01T17:06:31Z

@walles I don't know anything else but asking the reviewers (which I've done). Please understand that they are doing it voluntarily, using their free time.

src/main/java/redis/clients/jedis/JedisClusterCommand.java

walles · 2021-02-06T11:10:45Z

@yangbodong22011 / @sazzad16, with @yangbodong22011's suggested change applied (good catch Yang!), do you think this is ready for merge?

No behavior changes, just a refactoring. Changes: * Replaces recursion with a for loop * Extract redirection handling into its own method * Extract connection-failed handling into its own method Note that `tryWithRandomNode` is gone, it was never `true` so it and its code didn't survive the refactoring.

Inspired by redis#1334 where this went real easy :). Would have made redis#2355 shorter. Free public updates for JDK 7 ended in 2015: <https://en.wikipedia.org/wiki/Java_version_history> For JDK 8, free public support is available from non-Orace vendors until at least 2026 according to the same table. And JDK 8 is what Jedis is being tested on anyway: <https://github.com/redis/jedis/blob/ac0969315655180c09b8139c16bded09c068d498/.circleci/config.yml#L67-L74>

Co-authored-by: Yang Bodong <[email protected]>

mina-asham

Looks good, just suggesting a minor enhancement there but not a blocker for me.

src/main/java/redis/clients/jedis/JedisClusterCommand.java

Co-authored-by: Mina Asham <[email protected]>

src/main/java/redis/clients/jedis/JedisClusterCommand.java

Source (all of these point to the same place): * walles/retries-split * 4f80d73 * redis#2355

* Split JedisClusterCommand into multiple methods No behavior changes, just a refactoring. Changes: * Replaces recursion with a for loop * Extract redirection handling into its own method * Extract connection-failed handling into its own method Note that `tryWithRandomNode` is gone, it was never `true` so it and its code didn't survive the refactoring. * Drop redundant null check * Bump JDK version to 1.8 Inspired by #1334 where this went real easy :). Would have made #2355 shorter. Free public updates for JDK 7 ended in 2015: <https://en.wikipedia.org/wiki/Java_version_history> For JDK 8, free public support is available from non-Orace vendors until at least 2026 according to the same table. And JDK 8 is what Jedis is being tested on anyway: <https://github.com/redis/jedis/blob/ac0969315655180c09b8139c16bded09c068d498/.circleci/config.yml#L67-L74> * Replace ConnectionGetters with lambdas * Retrigger CI * Add backoff to Redis connections * Add unit tests for backoff logic * Add retries logging * Always use the user requested timeout * Remedy review feedback * Consider connection exceptions and disregard random nodes * consider connection exceptions and disregard random nodes * reset redirection * Revert "Consider connection exceptions and disregard random nodes" This reverts commit 67a062a. Lots of tests in JedisClusterCommandTests started failing, need to be fixed before trying again. * Add another backoff test case 1. We try to contact master => JedisConnectionException 2. We try to contact replica => It refers us to master, hasn't failed over yet 3. We try to contact master => JedisConnectionException 4. We try to contact replica => Success, because it has now failed over * consider connection exceptions and disregard random nodes * reset redirection * Fix test failure * Apply suggestions from code review Co-authored-by: Jens Green Olander <[email protected]> * update documentation * Improve a comment * Update src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java * Add change from another branch Source (all of these point to the same place): * walles/retries-split * 4f80d73 * #2355 * Move JedisClusterCommandTest out of commands package * Use JedisClusterOperationException * Reduce sleep time, especially when few attempts left * Update src/main/java/redis/clients/jedis/JedisClusterCommand.java * merge fix * merge fix * Use maxAttempts * format import * Re-add missing codes due to merge * avoid NPE while zero max attempts * Remove zero attempts test * More cluster constructors and customizability * Use maxTotalRetriesDuration everywhere * more missing maxTotalRetriesDuration after merge Co-authored-by: M Sazzadul Hoque <[email protected]> Co-authored-by: Jens Green Olander <[email protected]> Co-authored-by: Jens Green Olander <[email protected]>

sazzad16 suggested changes Jan 25, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

walles marked this pull request as ready for review January 25, 2021 09:19

walles mentioned this pull request Jan 25, 2021

Document JedisClusterCommand.releaseConnection() as Nullable #2356

Closed

walles mentioned this pull request Jan 25, 2021

Bump JDK version to 1.8 #2357

Closed

walles requested a review from sazzad16 January 25, 2021 10:24

sazzad16 previously approved these changes Jan 26, 2021

View reviewed changes

sazzad16 added the wait for more reviews label Jan 26, 2021

sazzad16 added this to the 3.6.0 milestone Jan 26, 2021

This was referenced Jan 27, 2021

Fix retries logic #2351

Closed

Retry with backoff on cluster connection failures #2358

Merged

sazzad16 requested review from mina-asham and yangbodong22011 January 29, 2021 10:26

walles mentioned this pull request Feb 1, 2021

JedisCluster retry caused java.lang.StackOverflowError #2173

Closed

yangbodong22011 reviewed Feb 5, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

walles dismissed sazzad16’s stale review via 0ad0b4f February 5, 2021 14:13

walles requested review from sazzad16 and yangbodong22011 February 5, 2021 16:59

Johan Walles and others added 6 commits February 8, 2021 09:12

Drop redundant null check

006731a

Replace ConnectionGetters with lambdas

8c5c0fe

Retrigger CI

4672751

Update src/main/java/redis/clients/jedis/JedisClusterCommand.java

4f138e4

Co-authored-by: Yang Bodong <[email protected]>

walles force-pushed the walles/retries-split branch from 0ad0b4f to 4f138e4 Compare February 8, 2021 08:12

mina-asham previously approved these changes Feb 17, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

walles dismissed mina-asham’s stale review via 31df601 February 17, 2021 13:24

walles requested a review from mina-asham February 17, 2021 13:24

mina-asham suggested changes Feb 17, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

Update src/main/java/redis/clients/jedis/JedisClusterCommand.java

4f80d73

Co-authored-by: Mina Asham <[email protected]>

walles force-pushed the walles/retries-split branch from 31df601 to 4f80d73 Compare February 17, 2021 14:19

walles requested a review from mina-asham February 17, 2021 14:20

sazzad16 reviewed Feb 17, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

sazzad16 reviewed Feb 17, 2021

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterCommand.java Outdated Show resolved Hide resolved

add last exception as suppressed

d305109

sazzad16 removed the wait for more reviews label Feb 17, 2021

walles pushed a commit to walles/jedis that referenced this pull request Feb 25, 2021

Add change from another branch

a23d602

Source (all of these point to the same place): * walles/retries-split * 4f80d73 * redis#2355

compatible for redis#2358

e62f584

sazzad16 merged commit 9006788 into redis:master Feb 26, 2021

walles deleted the walles/retries-split branch February 26, 2021 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split JedisClusterCommand into multiple methods #2355

Split JedisClusterCommand into multiple methods #2355

Uh oh!

walles commented Jan 25, 2021 •

edited

Loading

Uh oh!

sazzad16 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

walles commented Jan 26, 2021

Uh oh!

sazzad16 commented Jan 26, 2021

Uh oh!

walles commented Feb 1, 2021

Uh oh!

sazzad16 commented Feb 1, 2021

Uh oh!

Uh oh!

walles commented Feb 6, 2021

Uh oh!

mina-asham left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Split JedisClusterCommand into multiple methods #2355

Split JedisClusterCommand into multiple methods #2355

Uh oh!

Conversation

walles commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer notes

Uh oh!

sazzad16 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

walles commented Jan 26, 2021

Uh oh!

sazzad16 commented Jan 26, 2021

Uh oh!

walles commented Feb 1, 2021

Uh oh!

sazzad16 commented Feb 1, 2021

Uh oh!

Uh oh!

walles commented Feb 6, 2021

Uh oh!

mina-asham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

walles commented Jan 25, 2021 •

edited

Loading