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

Refactoring #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Refactoring #8

wants to merge 5 commits into from

Conversation

DivPro
Copy link

@DivPro DivPro commented Jun 26, 2019

features:

  1. fixing bug sentinel_tunnel stops working after several random host failures #7 (channel read lock)
  2. auth support posiibility(raw sockets replaced with go-redis)
  3. acceptance tests posiibility (docker-compose.yml + configs added)
  4. common architecture improvements

Copy link

@jmaitrehenry jmaitrehenry left a comment

Choose a reason for hiding this comment

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

Nice PR, I was looking for a redis proxy for my project and with this PR I think it will work!

return sentinels, nil
}

func createSentinel(addr string) (*redis.SentinelClient, error) {

Choose a reason for hiding this comment

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

It's impossible to return an error here, maybe remove the last return argument

Comment on lines +5 to +7
"github.com/DivPro/sentinel_tunnel/cmd/config"
"github.com/DivPro/sentinel_tunnel/cmd/resolver"
"github.com/DivPro/sentinel_tunnel/cmd/server"

Choose a reason for hiding this comment

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

Not the right package reference

Comment on lines +5 to +6
"github.com/DivPro/sentinel_tunnel/cmd/config"
"github.com/DivPro/sentinel_tunnel/cmd/resolver"

Choose a reason for hiding this comment

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

s/DivPro/RedisLabs

@DivPro
Copy link
Author

DivPro commented Oct 9, 2019

@jmaitrehenry Thank you for feedback, changes will coming soon,
and some more improvements such as context support

@jmaitrehenry
Copy link

@DivPro may I suggest to move the main to the root directory? Else, we could have some fun issues if you use the project outside your GOPATH

@DivPro
Copy link
Author

DivPro commented Oct 9, 2019

@jmaitrehenry wait please for changes, moving main => root approved

@indeyets
Copy link

indeyets commented Jun 3, 2020

@DivPro did you ever finish your changes?

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.

3 participants