RUST-933 Add support for the srvMaxHosts option #977
Conversation
| @@ -1721,6 +1750,26 @@ impl ConnectionString { | |||
| ConnectionStringParts::default() | |||
| }; | |||
|
|
|||
| if let Some(srv_max_hosts) = conn_str.srv_max_hosts { | |||
There was a problem hiding this comment.
Similarly, the spec says that the validation here has to happen both for the initial URI parse and after SRV resolution, so that gets duplicated too :/
| @@ -24,7 +24,7 @@ pub(crate) struct ResolvedConfig { | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub(crate) struct LookupHosts { | |||
| pub(crate) hosts: Vec<Result<ServerAddress>>, | |||
There was a problem hiding this comment.
This being a Vec<Result<ServerAddress>> rather than Vec<ServerAddress> with error values either being propagated or ignored depending on context made me stumble a bit when reading this code, so I switched it to the (IMO) more understandable simple value vec with an explicit behavior toggle.
| } | ||
| .into())); | ||
| continue; |
There was a problem hiding this comment.
Previously there was no continue here, which was kind of a nasty bug-in-waiting - invalid hosts would still have been added to the list.
src/client/options.rs
Outdated
| use rand::prelude::SliceRandom; | ||
| config.hosts.shuffle(&mut rand::thread_rng()); | ||
| config.hosts.truncate(max as usize); |
There was a problem hiding this comment.
The spec indicates that this should use the same method of random selection as server selection, but it looks like we do server selection differently:
Can we standardize these implementations and maybe add a helper that all three can use? (The spec also recommends using the fisher-yates algorithm specifically, but I'll defer to you on whether we should do that; I'd imagine that whatever
rand does is fine.)
There was a problem hiding this comment.
Done. And yeah, AFAICT rand's implementation of shuffle does actually use fisher-yates, but choose_multiple doesn't, but I think the idea is to make sure it's a valid and efficient selection, for which I'm happy to trust rand.
| address, | ||
| domain_name.join(".") | ||
| ), | ||
| if matches!(dm, DomainMismatch::Error) { |
There was a problem hiding this comment.
Based on the language in the spec (see the language in the second bullet here), it seems like we should never actually return an error here and instead log a warning. I think these errors were just being constructed for the sake of logging them at some point in the future? But I think it would be simpler to do the logging here directly rather than propagate the error. Based on that I think we can just remove this logic entirely and file a ticket to log a warning when this happens.
There was a problem hiding this comment.
When this is used as part of the initial discovery, the spec requires that it raise an error; for polling it must not raise an error. Previously this was implemented by the method always returning a Vec<Result<ServerAddress>> and then errors in that vec would either be propagated or ignored depending on which context was calling it; I changed that to have an explicit mode switch (the dm parameter) to make the behavior (imo) clearer and more of a direct analogue to the spec.
I missed that this should be logged, though; added code to log the message when tracing is enabled.
There was a problem hiding this comment.
ah I had missed that, sgtm
| } | ||
| .into())); | ||
| continue; |
abr-egn
force-pushed
the
RUST-933/srvMaxHosts
branch
from
7cfdbce to
25ee25c
Compare
| address, | ||
| domain_name.join(".") | ||
| ), | ||
| if matches!(dm, DomainMismatch::Error) { |
There was a problem hiding this comment.
ah I had missed that, sgtm
RUST-933
Pretty straightforward. The shuffling needs to be in two places (initial host discovery and polling), unfortunately, because the logic at those two places is a bit different.