GODRIVER-2092 Add srvMaxHosts URI option #764
Conversation
There was a problem hiding this comment.
Something feels off about including this option and SRVServiceName in the ClientOptions struct. Because SRV resolution is done in the ConnString code, a Client constructed with an SRV URI but SRVMaxHosts set in code options rather than the URI would not actually get the max hosts behavior. Thoughts on this?
| @@ -121,6 +121,7 @@ type ClientOptions struct { | |||
| ServerAPIOptions *ServerAPIOptions | |||
| ServerSelectionTimeout *time.Duration | |||
| SocketTimeout *time.Duration | |||
| SRVMaxHosts *int | |||
There was a problem hiding this comment.
Do we need to add code to ClientOptions#ApplyURI for this option?
There was a problem hiding this comment.
Let's add a test for this in clientoptions_test.go - we didn't catch this in the SRV polling tests because they construct a Topology directly, which is unfortunate. I'm hoping GODRIVER-1760 will help us clean up these abstractions and catch such issues quicker.
There was a problem hiding this comment.
Sounds good; done.
There was a problem hiding this comment.
If a user calls ClientOptions#ApplyURI after calling ClientOptions#SetSRVServiceName or ClientOptions#SetSRVMaxHosts, the SRV lookup for the initial DNS seedlist will take into account those options. I did document that caveat on the setters.
I think we need SRVServiceName and SRVMaxHosts on the ClientOptions struct to be able to pass them down to the Topology for SRV polling in client#configure(). I could remove the setters, but it seems odd to have an option and no setter on ClientOptions.
| @@ -121,6 +121,7 @@ type ClientOptions struct { | |||
| ServerAPIOptions *ServerAPIOptions | |||
| ServerSelectionTimeout *time.Duration | |||
| SocketTimeout *time.Duration | |||
| SRVMaxHosts *int | |||
There was a problem hiding this comment.
The documentation is sufficient for now. I don't love the limitation, but I also suspect that most people will be fine putting this in the URI. Maybe it's worth filing a ticket to move the SRV resolution logic into the Client so it's done after all of the ClientOptions stuff has been configured?
Regardless, LGTM mod the suggestion about adding unit tests for ClientOptions. No need to request another review, though.
| @@ -121,6 +121,7 @@ type ClientOptions struct { | |||
| ServerAPIOptions *ServerAPIOptions | |||
| ServerSelectionTimeout *time.Duration | |||
| SocketTimeout *time.Duration | |||
| SRVMaxHosts *int | |||
There was a problem hiding this comment.
Let's add a test for this in clientoptions_test.go - we didn't catch this in the SRV polling tests because they construct a Topology directly, which is unfortunate. I'm hoping GODRIVER-1760 will help us clean up these abstractions and catch such issues quicker.
|
Filed GODRIVER-2178 to move SRV resolution logic. |
GODRIVER-2092
Adds support for a new URI option
srvMaxHoststhat limits the number of hosts returned in initial DNS seedlist discovery and intermittent SRV polling. Syncsinitial-dns-seedlist-discoveryspec tests and implements new SRV polling prose tests.