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

feat: Option to disable dial on creating client #584

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SoulPancake
Copy link
Contributor

Approach to #489

@SoulPancake
Copy link
Contributor Author

SoulPancake commented Jul 1, 2024

Hi @rueian I decided to begin on this
However, I have a few doubts about the requirement and have some questions.

  1. feature request: adding an option to disable dial on creating client #489 (comment)
    In this how do we go about testing the last point ( Make sure all the above clients can work after redis is back. ) in a failsafe way.
  2. How does the auto-retry work? Do we mean simply returning the structs of these clients without the dial failure being involved like I have done in the current commit? If yes, then how do we update it when the retry is successful?

Thanks in advance. Let me know which direction I can take for this

@rueian
Copy link
Collaborator

rueian commented Jul 2, 2024

In this how do we go about testing the last point ( Make sure all the above clients can work after redis is back. ) in a failsafe way.

If you are asking how to write tests for this behavior, I think passing a custom DialFn function, which rejects the first few connections, to the rueidis.ClientOption can help.

How does the auto-retry work? Do we mean simply returning the structs of these clients without the dial failure being involved like I have done in the current commit? If yes, then how do we update it when the retry is successful?

I think, yes, returning their concrete structs is enough except for the sentinel client. The sentinel client may need some modifications to support auto-retry. But for all these three cases, the initial err should also be returned along with their concrete structs.

rueidis.go Outdated Show resolved Hide resolved
rueidis.go Outdated Show resolved Hide resolved
rueidis.go Outdated Show resolved Hide resolved

_, port, _ := net.SplitHostPort(ln.Addr().String())

dialFn := func(addr string, dialer *net.Dialer, tlsConfig *tls.Config) (net.Conn, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rueian Do you think this is how I should do it?
In principle it should work but I am not too sure why it is timing out, maybe I'm missing something

Copy link
Collaborator

Choose a reason for hiding this comment

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

The dailFn looks good to me but you must invoke client.Do to trigger the dailFn again after the NewClient. Otherwise, the dailFn is only called once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am slightly stuck here
I should client.Do to trigger the dialFn right after the NewClient call? @rueian

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I believe so. There is no other background goroutine that will dial for a connection.

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.

2 participants