PHPC-361: Manager::getServers() should omit unknown servers

[jmikola]

https://jira.mongodb.org/browse/PHPC-361

[bjori]

I'm pretty sure we decided against that.

getServers() was supposed to return you the currently known servers.
If there hasn't been any op yet, then we don't know of any...

[jmikola]

I didn't recall any discussion about it. Before this patch, the behavior I observed was that getServers() returns Server objects based on the hosts inferred from the URI itself. Those will have an unknown state and it's also possible that their hints won't be usable later on. I think users might be more baffled by the fact that we'd create Server objects for them that may not be usable.

This is distinct from the situation where we might return a Server for a replica set node that ends up being removed from the set (or shut down) before the user attempts to execute an operation on it directly. At least in that case, the Server object did have some known state and an isMaster record.

[bjori]

See https://jira.mongodb.org/browse/PHPC-194

You describe yourself the difference of selectServer() and getServers(), seemingly firm on getServers() behaving correctly.

[jmikola]

@bjori: To quote that ticket:

This is mainly because Manager construction is lazy and Manager::getServers() would return an empy array before initialization time.

At some point, this must have changed. intern->client->topology->description.servers might have been an empty set before, but it currently consists of one or more unknown server structs.

As an aside, such a server selection should be possible to implement in userland given a ReadPreference and manually calling Manager::getServers() and sorting through the info (i.e. isMaster results) and latency for all servers; however, that would require a userland implementation to kickstart SDAM with a ping if the collection of servers was empty.

I suppose this PR is essentially adding that SDAM kickstart process we'd need to implement a userland selection process.

[bjori]

Subject: Implement Manager::selectServer() method to wrap SDAM server selection

This may require two new features in libmongoc:

• Manually trigger SDAM initialization
• Allow querying SDAM's server selection and obtaining a server hint (outside of doing an actual operation)

And thats exactly what we did.

This was all argued from the standpoint of getServers() behaving correctly.
If getServers() now does half of this, does it then make sense to remove selectServer() ?
The $readPreference argument could be merged to getServers()?

[jmikola]

If getServers() now does half of this, does it then make sense to remove selectServer() The $readPreference argument could be merged to getServers()?

There's benefit in selectServer() returning a single Server and raising exceptions when server selection fails. That API was ideal for the needs of PHPLIB.

I do see how getServers() could be employed for userland server selection (e.g. with a custom round-robin algorithm), irrespective of whether the extension method took a read preference. That said, I see it more as a diagnostic tool, akin to getConnections() in the legacy driver. If we made it more like selectServer(), then there'd be the question of whether failing to find any Server(s) would throw or simply return an empty array.

[jmikola]

Per discussion in IRC, I've revised PHPC-361 to state that getServers() should be changed to merely filter out servers in unknown states instead of performing full discovery. This keeps the method passive and removes functional overlap with selectServer().

I'll revise this PR after the 1.0.0-alpha release.

[jmikola]

Ready to be reviewed again.

[bjori]

if only we had realized the usefulness of %r()%r for the MongoLog tests!

[bjori]

No mongos test?

lgtm

[derickr]

LGTM too, but have a look at that mongos test that @bjori mentions :-)

[jmikola]

Created PHPC-402 to look into the shard cluster testing. We don't currently have a persistent cluster environment in the MO VM.

As it relates to this PR, I don't see any reason why it wouldn't return mongos connections, so I'll merge as-is and we'll follow this up in the other ticket.