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

Use WithContextDialer to replace WithDialer #472

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

kerthcet
Copy link
Contributor

Cleanup for TODO.

@kerthcet
Copy link
Contributor Author

kerthcet commented Jan 2, 2024

@kerthcet kerthcet closed this Jan 2, 2024
@kerthcet kerthcet deleted the cleanup/diaer branch January 2, 2024 09:55
@elezar elezar self-assigned this Jan 28, 2024
@elezar elezar self-requested a review January 28, 2024 21:27
@elezar
Copy link
Member

elezar commented Jan 28, 2024

@kerthcet please reopen this. We are migrating to GitHub and I will review it here.

@kerthcet
Copy link
Contributor Author

/reopen

@kerthcet kerthcet restored the cleanup/diaer branch January 29, 2024 03:08
@kerthcet kerthcet reopened this Jan 29, 2024
//nolint:staticcheck
grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
return net.DialTimeout("unix", addr, timeout)
grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
Copy link
Member

Choose a reason for hiding this comment

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

One question I have: How does the change in default TCP timeout as described in https://pkg.go.dev/google.golang.org/grpc#WithContextDialer

Note: All supported releases of Go (as of December 2023) override the OS defaults for TCP keepalive time and interval to 15s. To enable TCP keepalive with OS defaults for keepalive time and interval, use a net.Dialer that sets the KeepAlive field to a negative value, and sets the SO_KEEPALIVE socket option to true from the Control field. For a concrete example of how to do this, see internal.NetDialerWithTCPKeepalive().

affect this change here?

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 think they're different, one is for detecting network liveness, another is for waiting for the network connection. WithDialer also uses WithContextDialer underlying.

Signed-off-by: kerthcet <kerthcet@gmail.com>
@elezar elezar merged commit 0464ebb into NVIDIA:main Jul 11, 2024
6 checks passed
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.

None yet

2 participants