Skip to content

PHPC-827: Update Max Staleness implementation #469

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 4 commits into from
Nov 28, 2016

Conversation

derickr
Copy link
Contributor

@derickr derickr commented Nov 27, 2016

@derickr derickr force-pushed the PHPC-827-update-max-staleness branch 2 times, most recently from 37bcbae to 4a0f8bc Compare November 27, 2016 18:12
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected maxStalenessSeconds to be <= %" PRId32 ", %" PHONGO_LONG_FORMAT " given", INT32_MAX, maxStalenessSeconds);
return;
}
if (maxStalenessSeconds > 0 && mode == MONGOC_READ_PRIMARY) {
Copy link
Member

Choose a reason for hiding this comment

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

This should check if it's not equal to NO_MAX_STALENESS.

}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY, null, ['maxStalenessMS' => -1]);
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY, null, ['maxStalenessSeconds' => -2]);
Copy link
Member

Choose a reason for hiding this comment

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

Let's add test cases for 0 (another boundary case) and 42 (something less than 90) as well.

}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

echo throws(function() {
$manager = new MongoDB\Driver\Manager(null, ['readPreference' => 'secondary', 'maxStalenessMS' => -1]);
$manager = new MongoDB\Driver\Manager(null, ['readPreference' => 'secondary', 'maxStalenessSeconds' => -2]);
Copy link
Member

Choose a reason for hiding this comment

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

Let's add test cases for 0 (another boundary case) and 42 (something less than 90) as well.

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.

LGTM with comparison fix (see comment) and additional invalid test cases.

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected maxStalenessSeconds to be <= %" PRId32 ", %" PHONGO_LONG_FORMAT " given", INT32_MAX, maxStalenessSeconds);
return;
}
if (maxStalenessSeconds != MONGOC_NO_MAX_STALENESS && mode == MONGOC_READ_PRIMARY) {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized this is redundant since we're already within a maxStalenessSeconds != MONGOC_NO_MAX_STALENESS conditional. No harm in leaving this in place, though. We can always refactor later.

derickr and others added 4 commits November 28, 2016 11:32
The expected output of bson-javascript-jsonserialize-004.phpt is logically incorrect because of an outstanding bug where Javascript objects are always serialized to BSON code types, irrespective of their scope property. That issue will be fixed by PHPC-838.
@derickr derickr force-pushed the PHPC-827-update-max-staleness branch from 46c2fe6 to 5c63b8d Compare November 28, 2016 11:32
@derickr derickr merged commit 5c63b8d into mongodb:master Nov 28, 2016
derickr added a commit that referenced this pull request Nov 28, 2016
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