Skip to content

Conversation

@sazzad16
Copy link
Contributor

@sazzad16 sazzad16 commented Feb 6, 2021

Resolves #1780
Resolves #1824
Resolves #2234

Closes #1787
Closes #2237

Closes #2342
Closes #2349
Closes #2352
Closes #2353
Closes #2363

Copy link
Contributor

@mina-asham mina-asham left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for going through the comments from the prevision revision! Just a few more minor comments here and then I am happy to sign off

mina-asham
mina-asham previously approved these changes Feb 17, 2021
@sazzad16 sazzad16 force-pushed the config-pattern-extend-3-2 branch from 22f3f0f to c542793 Compare February 26, 2021 14:04
@gkorland gkorland requested a review from mina-asham March 3, 2021 15:38
@sazzad16
Copy link
Contributor Author

sazzad16 commented Mar 4, 2021

@gkorland

  1. ShardedJedisPoolTest.shouldReturnActiveShardsWhenOneGoesOffline()
  2. ShardedJedisPoolWithCompleteCredentialsTest.shouldReturnActiveShardsWhenOneGoesOffline()
  3. ShardedJedisTest.testMasterSlaveShardingConsistency()
  4. ShardedJedisTest.testMasterSlaveShardingConsistencyWithShardNaming()

These tests used to work because underlying connections were not always created given the combination of parameters. All the Jedis classes were suffering from this situation. The underlying connection of object representing a connection should either always or NEVER be created from constructor. In this PR I implemented the "always" approach.

I was able to keep the tests with a valid host address in (3) and (4).

But I could not make (1) and (2) to work. Even though I @Ignored first, I removed those after review. A portion of (1) and (2):

    shards.set(1, new JedisShardInfo("localhost", 1234));
    pool = new ShardedJedisPool(redisConfig, shards);
    jedis = pool.getResource();

This is more logical that pool.getResource() would fail because 1234 is an invalid port. But the tests are expecting pool.getResource() without any error. That's why these two tests won't work with this PR.

IMHO, we shouldn't care about impractical tests. Still, I can @Ignore those if you prefer.

@sazzad16 sazzad16 requested a review from gkorland March 4, 2021 09:58
@sazzad16 sazzad16 merged commit 8896fea into redis:master Mar 5, 2021
@sazzad16 sazzad16 deleted the config-pattern-extend-3-2 branch May 18, 2021 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants