Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new feature to ping connection to a given URI #598

Closed
wants to merge 4 commits into from

Conversation

7snovic
Copy link
Contributor

@7snovic 7snovic commented May 26, 2017

Inspired by this issue @ SO , we need a simple way to test connection to a given URI , Also like the C++ driver.

there was two implementations to do this,

  1. Automatically check within the \Manager::__construct(); but that will enforce to add an additional operation to check that connection
  2. Create a separated method to check/ping connection to a given URI.

I went with the second choice to make it optional for the developer to whether check his connection or not

@jmikola @derickr

@derickr
Copy link
Contributor

derickr commented May 30, 2017

I think this is much better suited for the PHP Library instead: https://github.com/mongodb/mongo-php-library/

@7snovic
Copy link
Contributor Author

7snovic commented May 30, 2017

@derickr I thought about that before pushing this, also thought about pushing an alternative to the library, but what if for some reason someone needs to use the driver directly, this will might be helpful if it was in the driver -which represent the low-level so to speak- rather than the library.

@jmikola
Copy link
Member

jmikola commented May 30, 2017

I'm not familiar with the C++ driver's history for providing a ping helper, but this looks like it only attempts to connect to the primary server. Ideally, I expect it'd allow any available server to be selected (minimizing the chance of a server selection error) in order to guarantee the libmongoc client is initialized from the connection URI. Beyond that, I have a few thoughts on its relation to the PHP driver:

The current design of the extension objects to adding any command helpers. executeQuery() and executeBulkWrite() are technically command helpers for newer server versions, but older server versions use different protocols for query and write messages and we rely on libmongoc to handle that for us. In the interest of keeping things sane for our users, we don't force them to use executeCommand() for queries and writes.

Existing command helpers in PHPLIB mainly exist to handle API differences for command across server versions (e.g. option validation, wire protocol changes). The ping command is basically a one-liner for both the extension and library:

$manager->executeCommand('admin', new MongoDB\Command(['ping' => 1]));

$client->admin->command(['ping' => 1]);

Beyond the extension argument, I don't think the complexity above warrants the overhead of implementing a helper for PHPLIB (e.g. operation class, tests, documentation).

@7snovic
Copy link
Contributor Author

7snovic commented Jun 4, 2017

Yes Exactly, it's kind of a little helper

Ideally, I expect it'd allow any available server to be select....

However if I got what you mean, basically that helper pinging the provided URI to the manager


but older server versions use different ...

you mean that we need to move this forward the new server?

However feel free to close this if this PR violates the current design of the extension, or we may flag this as waiting-for-mongoc ?.

@7snovic
Copy link
Contributor Author

7snovic commented Jun 4, 2017

However the main issue here is that this :

$manager = new MongoDB\Driver\Manager('mongodb://bad_uri:27017', $uriOptions, $driverOptions);

will never throw any errors even though it's a bad parameter,

also in another context I can't see that we hit the connection exception within the extension in any place

@jmikola
Copy link
Member

jmikola commented Jun 5, 2017

However if I got what you mean, basically that helper pinging the provided URI to the manager

As implemented in the PR, the ping helper requires a primary server to be available. My suggestion would have been to use a less strict read preference (e.g. primaryPreferred, secondaryPreferred, nearest), which would allow the ping to be issues even if a primary server was not available. This read preference corresponds to the third parameter in MongoDB\Driver\Manager::executeCommand().


you mean that we need to move this forward the new server?

I was only explaining why the extension API includes executeQuery() and executeBulkWrite() instead of a single executeCommand() method. Although recent server versions implement queries and write operations on top of the same command API, older server versions required different protocols for those operations.


However feel free to close this if this PR violates the current design of the extension

That is indeed what I was getting at. The design of the extension does not allow for introducing command helpers, and I'd argue this is also not necessary for the userland library because it's a one-liner.


However the main issue here is that this :

$manager = new MongoDB\Driver\Manager('mongodb://bad_uri:27017', $uriOptions, $driverOptions);

will never throw any errors even though it's a bad parameter,

This is by design according to the server discovery and monitoring specification. Single-threaded clients should not do any IO in the constructor and connections are effectively initialized lazily for the first operation.

also in another context I can't see that we hit the connection exception within the extension in any place

Any operation that requires using of a connection would trigger an exception in both the extension and userland library. In the most limited form in the extension, this could be executing any query, command, or write operation:

$manager->executeCommand('admin', new MongoDB\Command(['ping' => 1]));

Alternatively, you could also trigger initialization of connections (and isMaster commands on each during topology discovery and monitoring) by invoking server selection with MongoDB\Driver\Manager::selectServer():

$manager->selectServer(new \MongoDB\Driver\ReadPreference(\MongoDB\Driver\ReadPreference::RP_NEAREST));

@jmikola jmikola closed this Jun 5, 2017
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.

None yet

3 participants