Skip to content

Refactoring#8

Open
DivPro wants to merge 5 commits into
RedisLabs:masterfrom
DivPro:refactoring
Open

Refactoring#8
DivPro wants to merge 5 commits into
RedisLabs:masterfrom
DivPro:refactoring

Conversation

@DivPro

@DivPro DivPro commented Jun 26, 2019

Copy link
Copy Markdown

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

@jmaitrehenry jmaitrehenry left a comment

Copy link
Copy Markdown

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!

Comment thread cmd/resolver/factory.go
return sentinels, nil
}

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

Copy link
Copy Markdown

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 thread cmd/main.go
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"

Copy link
Copy Markdown

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 thread cmd/server/server.go
Comment on lines +5 to +6
"github.com/DivPro/sentinel_tunnel/cmd/config"
"github.com/DivPro/sentinel_tunnel/cmd/resolver"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/DivPro/RedisLabs

@DivPro

DivPro commented Oct 9, 2019

Copy link
Copy Markdown
Author

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

@jmaitrehenry

Copy link
Copy Markdown

@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

DivPro commented Oct 9, 2019

Copy link
Copy Markdown
Author

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

@indeyets

indeyets commented Jun 3, 2020

Copy link
Copy Markdown

@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