Skip to content

Regression tests for find() and aggregate() using primary RP within transaction #564

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

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jul 12, 2018

See: #562

This is a regression test for CDRIVER-2744. It builds upon #563 (for PHPLIB-373) and may be fixed by either:

  • PHPLIB-375: omitting read preference option for commands and queries sent in non-sharded topologies
  • PHPC-1236: Bumping libmongoc to 1.11.1 (once released)

I don't think we should merge this while failing and would suggest leaving it open until one of the solutions can be implemented.

@jmikola jmikola requested a review from derickr July 12, 2018 21:08
@jmikola jmikola changed the title Regression tests cdriver 2744 Regression test for find() and primary RP within transaction Jul 12, 2018
@jmikola jmikola force-pushed the regression-tests-cdriver-2744 branch from 0172c9f to 8dcc578 Compare July 13, 2018 15:47
@jmikola jmikola changed the title Regression test for find() and primary RP within transaction Regression tests for find() and aggregate() using primary RP within transaction Jul 13, 2018
@@ -104,6 +106,40 @@ public function testGetNamespace()
$this->assertEquals($this->getNamespace(), $this->collection->getNamespace());
}

public function testAggregateWithinTransaction()
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to add tests for aggregate() in response to #562 (comment). Best I can tell, there are no problems with aggregate(), which jives with the underlying libmongoc bug.

@@ -13,7 +13,7 @@
"php": ">=5.5",
"ext-hash": "*",
"ext-json": "*",
"ext-mongodb": "^1.5.0"
"ext-mongodb": "^1.5.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

This ensures that CDRIVER-2744 is in place via libmongoc 1.12.

@jmikola
Copy link
Member Author

jmikola commented Oct 30, 2018

@derickr: This has been rebased on master, so please review at your leisure. I don't expect the ext-mongodb bump is controversial, since the master branch will likely become PHPLIB 1.5.0 and depend on PHPC 1.6.0.

// Collection must be created before the transaction starts
$options = ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)];
$operation = new CreateCollection($this->getDatabaseName(), $this->getCollectionName(), $options);
$operation->execute($this->getPrimaryServer());
Copy link
Contributor

Choose a reason for hiding this comment

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

These three lines are copied many times, perhaps they should go into a helper function in FunctionalTestCase?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll make the change and update the PR.

…ransaction

This bumps the PHPC requirement to 1.5.2+, since that includes libmongoc 1.12 and CDRIVER-2744.
@jmikola jmikola force-pushed the regression-tests-cdriver-2744 branch from 48ef262 to ba0b611 Compare November 2, 2018 21:43
@jmikola
Copy link
Member Author

jmikola commented Nov 2, 2018

@derickr: I created generic create/drop collection helpers that apply a majority WC when supported by the server. Testing locally, there was no problem using a majority WC with a standalone, but we'll see what Travis says.

I left some explicit uses of CreateCollection and DropCollection operations as-is, particularly when they were using custom collection names.

Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

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

Travis says OK, and so do I.

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