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

[issue 1094] connectionTimeout respects net.Dialer default timeout #1095

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Sep 15, 2023

Fixes #1094

Motivation

connection.go's net.Dialer gets timeout with the default setting occasionally. The default has been increased by PR #563 but I think increasing the pulsar-client-go library default is not the answer, instead we should respect net.Dialer's default.

Although the ConnectionTimeout can be a user specified value at the NewClient() creation, the default is 10 seconds that is hard coded in client_impl.go (https://github.com/apache/pulsar-client-go/blob/master/pulsar/client_impl.go#L34)

In fact, the previous value was 5 seconds. It was increased to 10 seconds by this PR #563

I believe we should not tweak Go's default, instead to respect the OS default. Here is the Go's net.Dialer comments. It states the TCP timeouts are often around 3 minutes. Ubuntu version I checked is at 2 minutes.

type Dialer struct {
	// Timeout is the maximum amount of time a dial will wait for
	// a connect to complete. If Deadline is also set, it may fail
	// earlier.
	//
	// The default is no timeout.
	//
	// When using TCP and dialing a host name with multiple IP
	// addresses, the timeout may be divided between them.
	//
	// With or without a timeout, the operating system may impose
	// its own earlier timeout. For instance, TCP timeouts are
	// often around 3 minutes.
	Timeout time.Duration

In the NON TLS dial, the same timeout is used. Go's net.DialTimeout states the timeout also includes name resolution, if it resolves to multiple IPs, the timeout is shared between each consecutive dial. This could result more time spent on dialing.

// DialTimeout acts like Dial but takes a timeout.
//
// The timeout includes name resolution, if required.
// When using TCP, and the host in the address parameter resolves to
// multiple IP addresses, the timeout is spread over each consecutive
// dial, such that each is given an appropriate fraction of the time
// to connect.
//
// See func Dial for a description of the network and address
// parameters.
func DialTimeout(network, address string, timeout time.Duration) (Conn, error) {

Therefore, I think the default of the client library should respect the OS setting. It means do not set the timeout if an application does not set it.

Modifications

Pass 0 value of time.Duration for ConnectionTimeout as the default for net.Dial

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as any connection created to a broker.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no )
  • The default values of configurations: (yes)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit 7cf643b into apache:master Sep 15, 2023
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.

The Dialer in the connection.go should respect OS TCP default
3 participants