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

DRIVERS-2577: add a runCommand specification #1389

Merged
merged 36 commits into from Apr 21, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Mar 28, 2023

Link to github generated document

Process meta:

  • DRIVERS-2577 is repurposed to both remove the RW concern options for the UTF and add the runCommand spec
  • A following PR will use the driver epic number (DRIVERS-2533) to track the new runCursorCommand spec

Adds a specification for the existing runCommand API. Generally, it summarizes requirements across the specifications and links the reader to the more detailed specification on the subject. Adds language that encourages drivers to not inspect nor modify user input, drivers will have to assess their current implementation and determine how that guidance can best be
followed.

Test coverage is targeted at each subject in the spec inspecting command construction is performed as expected. For example, a new test asserts that a database configured with a RC will not attach read concern fields to the user's command document per the spec.

Removing Read Concern and Write Concern settings from the runCommand unified test operation because they intentionally are not supported and no tests have been written to use them.

Please complete the following before merging:

@nbbeeken nbbeeken changed the title DRIVERS-2533: Expose an API to create a cursor from a command response [WIP] DRIVERS-2533: Expose an API to create a cursor from a command response Mar 28, 2023
@nbbeeken nbbeeken force-pushed the DRIVERS-2533-runCursorCommand branch 3 times, most recently from 172aa47 to 9d426c2 Compare April 3, 2023 18:43
@nbbeeken nbbeeken force-pushed the DRIVERS-2533-runCursorCommand branch 2 times, most recently from 967382e to 474833d Compare April 12, 2023 18:51
@nbbeeken nbbeeken changed the title [WIP] DRIVERS-2533: Expose an API to create a cursor from a command response [WIP] DRIVERS-2533: add a runCommand specification Apr 12, 2023
@nbbeeken nbbeeken force-pushed the DRIVERS-2533-runCursorCommand branch 3 times, most recently from fb87726 to 95900ac Compare April 12, 2023 18:59
@nbbeeken nbbeeken force-pushed the DRIVERS-2533-runCursorCommand branch from 95900ac to 50fdf4b Compare April 12, 2023 19:02
nbbeeken and others added 2 commits April 14, 2023 09:36
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@nbbeeken nbbeeken changed the title [WIP] DRIVERS-2533: add a runCommand specification DRIVERS-2533: add a runCommand specification Apr 14, 2023
@nbbeeken nbbeeken marked this pull request as ready for review April 14, 2023 13:57
@nbbeeken nbbeeken requested review from a team as code owners April 14, 2023 13:57
@nbbeeken nbbeeken requested review from jess-sig and jyemin and removed request for a team April 14, 2023 13:57
@nbbeeken nbbeeken requested a review from dariakp April 17, 2023 14:32
@nbbeeken nbbeeken requested review from jmikola and durran April 19, 2023 14:52
@jess-sig jess-sig removed their request for review April 19, 2023 18:56
@dariakp dariakp self-assigned this Apr 19, 2023
source/run-command/run-command.rst Outdated Show resolved Hide resolved
source/run-command/run-command.rst Outdated Show resolved Hide resolved
source/run-command/run-command.rst Outdated Show resolved Hide resolved
source/run-command/run-command.rst Outdated Show resolved Hide resolved
source/run-command/run-command.rst Outdated Show resolved Hide resolved
source/run-command/tests/unified/run-command.yml Outdated Show resolved Hide resolved
arguments:
client: *client
failPoint:
configureFailPoint: failCommand
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to add useMultipleMongoses: true in the client entity definition to avoid flaky test failures.

Per failPoint in the Unified Test Format spec:

The client entity SHOULD specify false for useMultipleMongoses if this operation could be executed on a sharded topology (according to runOnRequirements or test.runOnRequirements). This is advised because server selection rules for mongos could lead to unpredictable behavior if different servers were selected for configuring the fail point and executing subsequent operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you meant to say set it to false, per the spec right? changed it

Copy link
Member

Choose a reason for hiding this comment

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

Oy, typo on my part. Yes, I meant useMultipleMongoses: false.

source/run-command/tests/unified/run-command.yml Outdated Show resolved Hide resolved
Comment on lines 176 to 177
- topologies: [ replicaset, sharded-replicaset, load-balanced, sharded ]
minServerVersion: '4.4'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- topologies: [ replicaset, sharded-replicaset, load-balanced, sharded ]
minServerVersion: '4.4'
- minServerVersion: "4.0"
topologies: [ replicaset ]
- minServerVersion: "4.2"
topologies: [ sharded-replicaset, load-balanced ]

Not sure why minServerVersion: 4.4 was used, but other transaction tests allow 4.0+ replica sets and 4.2+ sharded clusters.

Specifying sharded-replicaset and sharded together also seemed redundant.

I don't think sharded should be used for any spec tests that require successfully executing a transaction since an oplog is needed. It looks like there are a few instances of tests requiring sharded instead of sharded-replicaset. I'll open a separate JIRA ticket about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, and thanks for clearing that up

Copy link
Member

Choose a reason for hiding this comment

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

Just to close the loop here, I created DRIVERS-2609 to clear up other spec tests.

source/run-command/tests/unified/run-command.yml Outdated Show resolved Hide resolved
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

I also POC-ed the tests in PHPLIB: mongodb/mongo-php-library#1066

@nbbeeken nbbeeken merged commit ab66eaf into master Apr 21, 2023
3 checks passed
@nbbeeken nbbeeken deleted the DRIVERS-2533-runCursorCommand branch April 21, 2023 13:29
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

5 participants