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.

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

issue #2153 - honor sentinel configuration params within MasterListener #2159

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

timmixell
Copy link
Contributor

@timmixell timmixell commented Mar 5, 2020

I could use some assistance/direction on proper unit test coverage, but I confirmed that the change addresses issue 2153 in our environment. Thank you!

Closes #2124
Resolves #2153

@sazzad16 sazzad16 added this to the 3.2.1 milestone Mar 5, 2020
@timmixell
Copy link
Contributor Author

My sincere apologies. I didn't realize that there were already open PRs addressing this issue when I submitted this yesterday. Upon closer inspection, it looks like this PR would handle the scenario more gracefully when the target Sentinel is unavailable (i.e. if j.auth() throws a JedisException). I'm pretty sure this PR should be closed out in favor of #2124.

@sazzad16
Copy link
Collaborator

sazzad16 commented Mar 6, 2020

@timmixell Could you please move all the changes including new Jedis object creation after if block of double check?

@sazzad16 sazzad16 requested a review from gkorland March 8, 2020 15:03
@gkorland
Copy link
Contributor

gkorland commented Mar 8, 2020

This file seems like it needs a full refactor

@gkorland
Copy link
Contributor

gkorland commented Mar 8, 2020

And it seems like we're missing a test to test this use case,

@timmixell
Copy link
Contributor Author

@gkorland , I'll get one in. I wasn't sure if this PR was going to survive, given that there are a couple other PRs addressing this issue.

@timmixell
Copy link
Contributor Author

@gkorland I'm struggling a bit on the best approach to write a unit test to support this use case. Do you have any suggestions? Thanks!

@sazzad16
Copy link
Collaborator

@gkorland Perhaps we can merge it right now and create a new task to write test(s). WDYT?

@sazzad16 sazzad16 modified the milestones: 3.2.1, 3.3.0 Mar 14, 2020
@sazzad16 sazzad16 merged commit e9949b3 into redis:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jedis MasterListener Sentinel listeners NOAUTH error
3 participants