Skip to content

[Issue 1094][doc] Enhance ConnectionTimeout/KeepAliveInterval comments#1488

Open
geniusjoe wants to merge 1 commit intoapache:masterfrom
geniusjoe:dev/update-timeout-comment
Open

[Issue 1094][doc] Enhance ConnectionTimeout/KeepAliveInterval comments#1488
geniusjoe wants to merge 1 commit intoapache:masterfrom
geniusjoe:dev/update-timeout-comment

Conversation

@geniusjoe
Copy link
Copy Markdown
Contributor

Fixes #1094 and #1095

Motivation

In Issue #1094, the default value of ConnectionTimeout was changed from 10s to 0 (falling back to the OS kernel's TCP timeout), but the corresponding GoDoc comment was not updated accordingly — it still stated "default: 5 seconds". This stale comment is misleading and can cause confusion for users trying to understand the actual connection behavior.

Additionally, the existing comments for both ConnectionTimeout and KeepAliveInterval lack sufficient detail about their respective roles and how they interact during connection lifecycle management (TCP dial phase vs. post-connection liveness detection via Ping/Pong), making it difficult for users to properly tune reconnection behavior.

Modifications

  • pulsar/client.go: Fixed the stale ConnectionTimeout default value in the comment, and enhanced the GoDoc for both ConnectionTimeout and KeepAliveInterval to clarify their roles, default behaviors, and interaction during reconnection.
  • pulsar/internal/helper.go: Added test helper functions to expose connectionTimeout and keepAliveInterval from the connection pool.
  • pulsar/client_impl_test.go: Added unit tests to verify the default values of ConnectionTimeout (0) and KeepAliveInterval (30s).

Verifying this change

This change added tests and can be verified as follows:

  • Added TestDefaultConnectionTimeout to verify the default ConnectionTimeout is 0 (no application-level timeout).
  • Added TestDefaultKeepAliveInterval to verify the default KeepAliveInterval is 30s.

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

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

Documentation

  • Does this pull request introduce a new feature? no

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

1 participant