Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/integration/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ type slowConnDialer struct {
}

var slowConnDialerDelay = 300 * time.Millisecond
var reducedHeartbeatInterval = 100 * time.Millisecond
var reducedHeartbeatInterval = 500 * time.Millisecond

func newSlowConnDialer(delay time.Duration) *slowConnDialer {
return &slowConnDialer{
Expand Down Expand Up @@ -516,7 +516,7 @@ func TestClient(t *testing.T) {
mt.Parallel()

// Reset the client with a dialer that delays all network round trips by 300ms and set the
// heartbeat interval to 100ms to reduce the time it takes to collect RTT samples.
// heartbeat interval to 500ms to reduce the time it takes to collect RTT samples.
mt.ResetClient(options.Client().
SetDialer(newSlowConnDialer(slowConnDialerDelay)).
SetHeartbeatInterval(reducedHeartbeatInterval))
Expand Down Expand Up @@ -554,7 +554,7 @@ func TestClient(t *testing.T) {
assert.Nil(mt, err, "Ping error: %v", err)

// Reset the client with a dialer that delays all network round trips by 300ms and set the
// heartbeat interval to 100ms to reduce the time it takes to collect RTT samples.
// heartbeat interval to 500ms to reduce the time it takes to collect RTT samples.
tpm := eventtest.NewTestPoolMonitor()
mt.ResetClient(options.Client().
SetPoolMonitor(tpm.PoolMonitor).
Expand Down
2 changes: 1 addition & 1 deletion internal/integration/unified/unified_spec_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ var (
}

logMessageValidatorTimeout = 10 * time.Millisecond
lowHeartbeatFrequency = 50 * time.Millisecond
lowHeartbeatFrequency = 500 * time.Millisecond
)

// TestCase holds and runs a unified spec test case
Expand Down
2 changes: 1 addition & 1 deletion internal/integration/unified_spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const (
)

var (
defaultHeartbeatInterval = 50 * time.Millisecond
defaultHeartbeatInterval = 500 * time.Millisecond
Copy link
Member

Choose a reason for hiding this comment

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

What is the history of the original 50ms default? Does the 500ms requirement conflict with legacy test specifications? Or was this value chosen arbitrarily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it came from this commit to implement streaming heartbeat protocol, not sure how the value was chosen though.

skippedTestDescriptions = map[string]string{
// SPEC-1403: This test checks to see if the correct error is thrown when auto encrypting with a server < 4.2.
// Currently, the test will fail because a server < 4.2 wouldn't have mongocryptd, so Client construction
Expand Down
8 changes: 7 additions & 1 deletion mongo/options/clientoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,11 @@ func (c *ClientOptionsBuilder) Validate() error {
}
}

if args.HeartbeatInterval != nil && *args.HeartbeatInterval < (500*time.Millisecond) {
return fmt.Errorf("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=%q",
*args.HeartbeatInterval)
}

if args.MaxPoolSize != nil && args.MinPoolSize != nil && *args.MaxPoolSize != 0 &&
*args.MinPoolSize > *args.MaxPoolSize {
return fmt.Errorf("minPoolSize must be less than or equal to maxPoolSize, got minPoolSize=%d maxPoolSize=%d",
Expand Down Expand Up @@ -755,7 +760,8 @@ func (c *ClientOptionsBuilder) SetDirect(b bool) *ClientOptionsBuilder {
}

// SetHeartbeatInterval specifies the amount of time to wait between periodic background server checks. This can also be
// set through the "heartbeatIntervalMS" URI option (e.g. "heartbeatIntervalMS=10000"). The default is 10 seconds.
// set through the "heartbeatFrequencyMS" URI option (e.g. "heartbeatFrequencyMS=10000"). The default is 10 seconds.
// The minimum is 500ms.
func (c *ClientOptionsBuilder) SetHeartbeatInterval(d time.Duration) *ClientOptionsBuilder {
c.Opts = append(c.Opts, func(opts *ClientOptions) error {
opts.HeartbeatInterval = &d
Expand Down
54 changes: 54 additions & 0 deletions mongo/options/clientoptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,60 @@ func TestClientOptions(t *testing.T) {
})
}
})
t.Run("heartbeatFrequencyMS validation", func(t *testing.T) {
testCases := []struct {
name string
opts *ClientOptionsBuilder
err error
}{
{
"heartbeatFrequencyMS > minimum (500ms)",
Client().SetHeartbeatInterval(10000 * time.Millisecond),
nil,
},
{
"heartbeatFrequencyMS == minimum (500ms)",
Client().SetHeartbeatInterval(500 * time.Millisecond),
nil,
},
{
"heartbeatFrequencyMS < minimum (500ms)",
Client().SetHeartbeatInterval(10 * time.Millisecond),
errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"10ms\""),
},
{
"heartbeatFrequencyMS == 0",
Client().SetHeartbeatInterval(0 * time.Millisecond),
errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"0s\""),
},
{
"heartbeatFrequencyMS > minimum (500ms) from URI",
Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=10000"),
nil,
},
{
"heartbeatFrequencyMS == minimum (500ms) from URI",
Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=500"),
nil,
},
{
"heartbeatFrequencyMS < minimum (500ms) from URI",
Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=10"),
errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"10ms\""),
},
{
"heartbeatFrequencyMS == 0 from URI",
Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=0"),
errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"0s\""),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.opts.Validate()
assert.Equal(t, tc.err, err)
})
}
})
t.Run("minPoolSize validation", func(t *testing.T) {
testCases := []struct {
name string
Expand Down
16 changes: 8 additions & 8 deletions x/mongo/driver/topology/polling_srv_records_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ func compareHosts(t *testing.T, received []description.Server, expected []string

func TestPollingSRVRecordsSpec(t *testing.T) {
for _, uri := range []string{
"mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100",
"mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500",
// Test with user:pass as a regression test for GODRIVER-2620
"mongodb+srv://user:[email protected]/?heartbeatFrequencyMS=100",
"mongodb+srv://user:[email protected]/?heartbeatFrequencyMS=500",
} {
t.Run(uri, func(t *testing.T) {
testPollingSRVRecordsSpec(t, uri)
Expand Down Expand Up @@ -169,7 +169,7 @@ func testPollingSRVRecordsSpec(t *testing.T, uri string) {

func TestPollSRVRecords(t *testing.T) {
t.Run("Not unknown or sharded topology", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -207,7 +207,7 @@ func TestPollSRVRecords(t *testing.T) {

})
t.Run("Failed Hostname Verification", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand All @@ -234,7 +234,7 @@ func TestPollSRVRecords(t *testing.T) {

})
t.Run("Return to polling time", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -276,7 +276,7 @@ func TestPollingSRVRecordsLoadBalanced(t *testing.T) {
}

t.Run("pollingRequired is set to false", func(t *testing.T) {
topo := createLBTopology(t, "mongodb+srv://test24.test.build.10gen.cc/?heartbeatFrequencyMS=100")
topo := createLBTopology(t, "mongodb+srv://test24.test.build.10gen.cc/?heartbeatFrequencyMS=500")
assert.False(t, topo.pollingRequired, "expected SRV polling to not be required, but it is")
})

Expand Down Expand Up @@ -313,7 +313,7 @@ func TestPollSRVRecordsMaxHosts(t *testing.T) {
simulateSRVPoll := func(srvMaxHosts int, recordsToAdd []*net.SRV, recordsToRemove []*net.SRV) (*Topology, func(ctx context.Context) error) {
t.Helper()

uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri).SetSRVMaxHosts(srvMaxHosts), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -385,7 +385,7 @@ func TestPollSRVRecordsServiceName(t *testing.T) {
simulateSRVPoll := func(srvServiceName string, recordsToAdd []*net.SRV, recordsToRemove []*net.SRV) (*Topology, func(ctx context.Context) error) {
t.Helper()

uri := "mongodb+srv://test22.test.build.10gen.cc/?heartbeatFrequencyMS=100&srvServiceName=customname"
uri := "mongodb+srv://test22.test.build.10gen.cc/?heartbeatFrequencyMS=500&srvServiceName=customname"
cfg, err := NewConfig(options.Client().ApplyURI(uri).SetSRVServiceName(srvServiceName), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down
Loading