Skip to content

Conversation

@prashantmital
Copy link
Contributor

@prashantmital prashantmital commented Jun 25, 2018

Closes PYTHON-1597

This allows us to specify (with the help of a decorator on the ClientContext) the storage engine that the server must be running with for a given test to be executed.

Motivation

Spec-tests introduced in PYTHON-1565 can only be run when the server is using wired Tiger. These tests failed when the MMAPv1 engine was in use, resulting in failures on evergreen.

Comments

I chose to use the db.serverStatus() command to obtain storage engine information because:

  1. other drivers already follow this approach in their test infrastructure (e.g. PHP)
  2. inferring storage engine information from the output of serverStatus seemed to present fewer ambiguities than the output of getCmdLineOpts. The latter tends to be confusing because the getCmdLineOpts['parsed'] dict seems to always contain the wiredTiger key and we need to check whether the engine key has been set to mmapv1 to ascertain the storage engine. This seemed a bit counter-intuitive to me but if you're not a fan of calling serverStatus, I can use the cmd_line information as well.

@prashantmital prashantmital force-pushed the enh/add-test-runner-helper-for-storage-engine branch from 22e6bee to bd8ec69 Compare June 25, 2018 19:14
@prashantmital prashantmital requested a review from behackett June 25, 2018 19:35
test/__init__.py Outdated

# May not have this if OperationFailure was raised earlier.
self.cmd_line = self.client.admin.command('getCmdLineOpts')
self.server_status = self.client.admin.command('serverStatus')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only need to run serverStatus once after creating the authenticated client. Just un-indent this line and remove the serverStatus on line 229.

test/__init__.py Outdated
"""Run a test only if the server is running the specified storage
engine (as determined by the `db.serverStatus` command)."""
#server_status = self.client.admin.command("serverStatus")
#current_engine = server_status["storageEngine"]["name"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comments.

test/__init__.py Outdated
#server_status = self.client.admin.command("serverStatus")
#current_engine = server_status["storageEngine"]["name"]
return self._require(
lambda: engine == self.server_status["storageEngine"]["name"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail when self.server_status is None. What if we just keep track of the storage engine instead of the entire server status response?


@classmethod
@client_context.require_version_min(4, 0, 0, -1)
@client_context.require_storage_engine("wiredTiger")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change streams work of storage engines other than wiredTiger. Like inMemory for example. Can you make these @client_context.require_not_mmap instead?

@prashantmital
Copy link
Contributor Author

Ignoring your comments for now as this will need a rethink since it cannot presently work with a sharded cluster.

@ShaneHarvey
Copy link
Member

Ignoring your comments for now as this will need a rethink since it cannot presently work with a sharded cluster.

For now we don't need it to work on a sharded cluster. All we need is to skip the spec tests when connected to a standalone server running with MMAP.

@ShaneHarvey ShaneHarvey merged commit f82706c into mongodb:master Jun 26, 2018
@ShaneHarvey
Copy link
Member

Thanks!

@prashantmital prashantmital deleted the enh/add-test-runner-helper-for-storage-engine branch June 26, 2018 17:31
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.

2 participants