Skip to content

Conversation

@juliusgeo
Copy link
Contributor

No description provided.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Are there any docs or changelog that should go along with this change?

res = self.explain.watch(pipeline=[{"$project": {"tags": 1}}],
batch_size=10, full_document="updateLookup")
self.assertIn("queryPlanner", res["stages"][0]["$cursor"])
self.assertTrue(self._recursiveIn("queryPlanner", res))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually prefer the previous behavior here because res["stages"][0]["$cursor"] helps us understand the structure of the explain response. _recursiveIn makes the command response more of a black box. Can you revert these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.assertTrue(self._recursiveIn("executionStats", res))
self.assertTrue(self._recursiveIn("allPlansExecution", res))

def test_comment(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of checking the explain result, this test should probably assert that we sent the comment field with the explain comment using a CommandListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

command_son = command.get_SON()
explain_command = SON([("explain", command_son)])
explain_command["verbosity"] = "queryPlanner"
explain_command["verbosity"] = self.verbosity or "queryPlanner"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest setting the default verbosity in __init__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.last_cmd_payload = command_son
return self.collection.database.command(explain_command)

def update_settings(self, verbosity=None, comment=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

In PyMongo the Collection object is immutable (the internal settings are configured once at creation and never changed). ExplainableCollection should probably be immutable too to match pymongo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

last_cmd_payload = self.explain.last_cmd_payload
self._compare_command_dicts(last_cmd_payload, last_logger_payload)

@unittest.skip("Travis does not have replica sets set up yet")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


explain = ExplainableCollection(collection, verbosity="queryPlanner",
comment="I'm a comment")
For more information see the documentation for the explain_ command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there needs to be a blank line after the code block.

self.assertIn("queryPlanner", res["stages"][0]["$cursor"])
self.collection.watch(pipeline=[{"$project": {"tags": 1}}],
batch_size=10, full_document="updateLookup")
batch_size=10, full_document="updateLookup")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change seems off batch_size should align with pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

last_logger_payload = self.logger.cmd_payload
res = self.explain.watch(pipeline=[{"$project": {"tags": 1}}],
batch_size=10, full_document="updateLookup")
batch_size=10, full_document="updateLookup")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.assertNotIn("comment", self.logger.cmd_payload)
self.explain = ExplainCollection(self.collection, comment="hey, "
"I'm a "
"comment")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the string format here. 3 short strings on new lines makes this harder to read. How about:

self.explain = ExplainCollection(self.collection, comment="comment")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@juliusgeo juliusgeo merged commit 3bcd0f1 into mongodb-labs:master Dec 14, 2021
@ShaneHarvey ShaneHarvey mentioned this pull request Jan 6, 2022
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.

3 participants