zzzming opened a new pull request, #1095: URL: https://github.com/apache/pulsar-client-go/pull/1095
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 https://github.com/apache/pulsar-client-go/pull/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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
