PHPC-1179: Reimplement tests that start servers with Mongo Orchestration

[derickr]

These tests could not have worked since we moved from a Wire Protocol getMore OP to the OP_COMMAND style of calling getMore. Beats me how we didn't manage to detect this.

The second commit isn't strictly necessary, but it updates the code to be able to throw a ConnectionException if getMore fails.

Previously, the getMore tests use a failPoint for the getMore OP, where the driver would then (through libmongoc) throw a ConnectionException. With the shift to the command-based failPoint, libmongoc/the driver now converted that to a ServerException. This second commit restores that behaviour. Alternatively, we can just update the test to check for a ServerException instead.

[derickr]

We use 237 here, as that's the same original code that MongoD would throw if a cursor had already gone by the time we call getMore. This allows us to make things consistent with the getMore OP behaviour from previous mongod versions. An errorCode is required here for the failPoint to work.

[jmikola]

This seems like something worth adding to a block comment for this line or method.

[derickr]

Done.

[jmikola]

If I understand correctly, a ConnectionException was previously thrown because we experienced an opcode failure instead of a mere command failure (with ok: 0). I understand the desire for consistency in error reporting, but are we lying to users here?

I would think that a ConnectionException implies that the socket itself is discarded. It's possible that libmongoc may very well have done that for the old getmore failure (I don't know offhand so it bears investigation), but I don't think it would for a failed command.

Both of these errors seem very much like ServerExceptions. Although both ConnectionException and ServerException extend RuntimeException (and may report error labels), users can now rest assured that these error codes come from the server, which makes ServerException more sensible.

With the older getmore opcode failures, there was no server-side error code, so ConnectionException (or a base RuntimeException) would have made sense.

[derickr]

That's why I had a separate commit for just this change. I think being more consistent is more helpful though...

[jmikola]

But this comes at a cost of inconsistency with regard to error code reporting. We tell users that that any server-originating error codes are used in exceptions within the ServerException hierarchy.

Am I correct in assuming that the error codes for killed cursors using OP_GETMORE differ from the command's error code? I believe so, as MONGOC_ERROR_CURSOR_INVALID_CURSOR was simply a sequential value in mongoc_error_code_t and that is what is used in _mongoc_rpc_check_ok() when inspecting an OP_REPLY. When the getMore command used, libmongoc now reports the error differently and uses the new server-originating error codes, which also changes how users can detect and differentiate each error.

IMO, the error reporting inconsistency is due to the server's API. Given that, I think the variation of exceptions is excusable based on the wire protocol we're using, and using ServerException is actually an improvement when it comes to distinguishing the errors via their code.

[derickr]

I've removed this now, and split the two test cases in a pre-3.6 and post-3.6 version.

[jmikola]

If you're now skipping tests when you know a failpoint isn't available, wouldn't you still want to keep the original behavior here (var_dump() and throw on unexpected failure to set a failpoint)?

[derickr]

No.

[jmikola]

Is it a programming error to call configureFailPoint() without first checking that the fail point is supported? I thought that's the reason we have the various skip functions and special behavior like reporting that failpoints are never supported on mongos.

[derickr]

It's complicated. Because we need to set two failPoints for getMore, which one is available depends on the exact mongod version. We can either ignore the exception, or, do lots of more complex checking in failGetMore. I've decided that this now "good enough" for what we need.

[jmikola]

Ah, I didn't notice the code for failGetMore() when I left that last comment (assuming it was implemented as such the entire time this PR has been open).

[jmikola]

Now that failGetMore() only calls itself when setting a fail point should succeed, can we get rid of the exception swallowing? That was one of the reasons for my suggestion to change failGetMore() in the first place.

[jmikola]

I reckon we can do away with this method entirely and simply define the appropriate constants in basic.inc as we do for others. Consider:

define('MONGODB_ORCHESTRATION_HOST', getenv('MONGODB_ORCHESTRATION_HOST') ?: 'localhost');
define('MONGODB_ORCHESTRATION_PORT', getenv('MONGODB_ORCHESTRATION_HOST') ?: '8889');
define('MONGODB_ORCHESTRATION_URI', sprintf('http://%s:%s/v1', MONGODB_ORCHESTRATION_HOST, MONGODB_ORCHESTRATION_PORT));

If the individual host and port values aren't needed as their own constant, you could also use local variables with the same ?: expression purely to ensure MONGODB_ORCHESTRATION_URI gets set properly.

[derickr]

There is no reason to have HOST and PORT as specific constants. And I also think that the name MONGODB_ORCHESTRATION_URI is wrong, as the "DB" isn't part of the "Mongo Orchestration" name. I've updated it.

[jmikola]

This seems like something worth adding to a block comment for this line or method.

[jmikola]

Ah, I didn't notice the code for failGetMore() when I left that last comment (assuming it was implemented as such the entire time this PR has been open).