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.

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

send AUTH message if sentinel password is set #2154

Merged
merged 7 commits into from Apr 14, 2020
Merged

send AUTH message if sentinel password is set #2154

merged 7 commits into from Apr 14, 2020

Conversation

kkolanow
Copy link
Contributor

Hi, I've noticed that during connection to the password protected sentinel (assume that master, replica slaves and sentinels share same password) I was getting error NOAUTH Authorization required. I discovered that sentinel password was not passed on. I'm submitting a fix example . I've tried it and it worked for me.

@sazzad16
Copy link
Collaborator

@kkolanow There are several errors in JedisSentinelPoolTest. Please check the Travis CI log.

@kkolanow
Copy link
Contributor Author

this time is fails on SetCommandsTest. I'm afraid I'm not sure what this test is supposed to do. Any help appreciated. Thanks!

sazzad16
sazzad16 requested changes Feb 28, 2020
View reviewed changes
Copy link
Collaborator

@sazzad16 sazzad16 left a comment •
edited

Choose a reason for hiding this comment

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

Only the first constructor can be added for user's convenience. Instead of using other new constructors, a user directly use the last constructor with all the params.
Also, don't do format only changes that are in distant lines from intended changes.

src/main/java/redis/clients/jedis/JedisSentinelPool.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/JedisSentinelPool.java Outdated Show resolved Hide resolved
@sazzad16 sazzad16 added this to the 3.3.0 milestone Feb 28, 2020
kkolanow and others added 2 commits February 28, 2020 21:05
@kkolanow @sazzad16
Update src/main/java/redis/clients/jedis/JedisSentinelPool.java
8d956ae 
code review update

Co-Authored-By: M Sazzadul Hoque <sazzad16@users.noreply.github.com>
@kkolanow @sazzad16
Update src/main/java/redis/clients/jedis/JedisSentinelPool.java
6776af3 
code review update

Co-Authored-By: M Sazzadul Hoque <sazzad16@users.noreply.github.com>
@gkorland
Copy link
Contributor

Now that the ACL is in @kkolanow @sazzad16 I think the code should adapted accordingly

@sazzad16 sazzad16 merged commit 07575b8 into redis:master Apr 14, 2020
@comphead comphead mentioned this pull request May 19, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sazzad16 sazzad16 sazzad16 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kkolanow @sazzad16 @gkorland @kkolanowski