-
Notifications
You must be signed in to change notification settings - Fork 209
PHPC-987: Causally consistent reads #710
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
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --TEST-- | ||
| Causal consistency: new session has no operation time | ||
| --SKIPIF-- | ||
| <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
| <?php NEEDS('REPLICASET'); ?> | ||
| --FILE-- | ||
| <?php | ||
| require_once __DIR__ . "/../utils/basic.inc"; | ||
|
|
||
| $manager = new MongoDB\Driver\Manager(REPLICASET); | ||
| $session = $manager->startSession(); | ||
|
|
||
| echo "Initial operation time:\n"; | ||
| var_dump($session->getOperationTime()); | ||
|
|
||
| ?> | ||
| ===DONE=== | ||
| <?php exit(0); ?> | ||
| --EXPECT-- | ||
| Initial operation time: | ||
| NULL | ||
| ===DONE=== |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| --TEST-- | ||
| Causal consistency: first read in session does not include afterClusterTime | ||
| --SKIPIF-- | ||
| <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
| <?php NEEDS('REPLICASET'); ?> | ||
| --FILE-- | ||
| <?php | ||
| require_once __DIR__ . "/../utils/basic.inc"; | ||
| require_once __DIR__ . "/../utils/observer.php"; | ||
|
|
||
| (new CommandObserver)->observe( | ||
| function() { | ||
| $manager = new MongoDB\Driver\Manager(REPLICASET); | ||
| $session = $manager->startSession(); | ||
|
|
||
| $query = new MongoDB\Driver\Query([]); | ||
| $manager->executeQuery(NS, $query, ['session' => $session]); | ||
| }, | ||
| function(stdClass $command) | ||
| { | ||
| $hasAfterClusterTime = isset($command->readConcern->afterClusterTime); | ||
| printf("Read includes afterClusterTime: %s\n", ($hasAfterClusterTime ? 'yes' : 'no')); | ||
| } | ||
| ); | ||
|
|
||
| ?> | ||
| ===DONE=== | ||
| <?php exit(0); ?> | ||
| --EXPECT-- | ||
| Read includes afterClusterTime: no | ||
| ===DONE=== |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| --TEST-- | ||
| Causal consistency: first read or write in session updates operationTime | ||
| --SKIPIF-- | ||
| <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
| <?php NEEDS('REPLICASET'); CLEANUP(REPLICASET); ?> | ||
| --FILE-- | ||
| <?php | ||
| require_once __DIR__ . "/../utils/basic.inc"; | ||
|
|
||
| class Test implements MongoDB\Driver\Monitoring\CommandSubscriber | ||
| { | ||
| private $lastSeenOperationTime; | ||
|
|
||
| public function executeBulkWrite() | ||
| { | ||
| $this->lastSeenOperationTime = null; | ||
|
|
||
| MongoDB\Driver\Monitoring\addSubscriber($this); | ||
|
|
||
| $manager = new MongoDB\Driver\Manager(REPLICASET); | ||
| $session = $manager->startSession(); | ||
|
|
||
| $bulk = new MongoDB\Driver\BulkWrite; | ||
| $bulk->insert(['x' => 1]); | ||
| $manager->executeBulkWrite(NS, $bulk, ['session' => $session]); | ||
|
|
||
| printf("Session reports last seen operationTime: %s\n", ($session->getOperationTime() == $this->lastSeenOperationTime) ? 'yes' : 'no'); | ||
|
|
||
| MongoDB\Driver\Monitoring\removeSubscriber($this); | ||
| } | ||
|
|
||
| public function executeCommand() | ||
| { | ||
| $this->lastSeenOperationTime = null; | ||
|
|
||
| MongoDB\Driver\Monitoring\addSubscriber($this); | ||
|
|
||
| $manager = new MongoDB\Driver\Manager(REPLICASET); | ||
| $session = $manager->startSession(); | ||
|
|
||
| $command = new MongoDB\Driver\Command(['ping' => 1]); | ||
| $manager->executeCommand(DATABASE_NAME, $command, ['session' => $session]); | ||
|
|
||
| printf("Session reports last seen operationTime: %s\n", ($session->getOperationTime() == $this->lastSeenOperationTime) ? 'yes' : 'no'); | ||
|
|
||
| MongoDB\Driver\Monitoring\removeSubscriber($this); | ||
| } | ||
|
|
||
| public function executeQuery() | ||
| { | ||
| $this->lastSeenOperationTime = null; | ||
|
|
||
| MongoDB\Driver\Monitoring\addSubscriber($this); | ||
|
|
||
| $manager = new MongoDB\Driver\Manager(REPLICASET); | ||
| $session = $manager->startSession(); | ||
|
|
||
| $query = new MongoDB\Driver\Query([]); | ||
| $manager->executeQuery(NS, $query, ['session' => $session]); | ||
|
|
||
| printf("Session reports last seen operationTime: %s\n", ($session->getOperationTime() == $this->lastSeenOperationTime) ? 'yes' : 'no'); | ||
|
|
||
| MongoDB\Driver\Monitoring\removeSubscriber($this); | ||
| } | ||
|
|
||
| public function commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event) | ||
| { | ||
| } | ||
|
|
||
| public function commandSucceeded(MongoDB\Driver\Monitoring\CommandSucceededEvent $event) | ||
| { | ||
| $reply = $event->getReply(); | ||
| $hasOperationTime = isset($reply->{'operationTime'}); | ||
|
|
||
| printf("%s command reply includes operationTime: %s\n", $event->getCommandName(), $hasOperationTime ? 'yes' : 'no'); | ||
|
|
||
| if ($hasOperationTime) { | ||
| $this->lastSeenOperationTime = $reply->operationTime; | ||
| } | ||
| } | ||
|
|
||
| public function commandFailed(MongoDB\Driver\Monitoring\CommandFailedEvent $event) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| echo "Testing executeBulkWrite()\n"; | ||
| (new Test)->executeBulkWrite(); | ||
|
|
||
| echo "\nTesting executeCommand()\n"; | ||
| (new Test)->executeCommand(); | ||
|
|
||
| echo "\nTesting executeQuery()\n"; | ||
| (new Test)->executeQuery(); | ||
|
|
||
| ?> | ||
| ===DONE=== | ||
| <?php exit(0); ?> | ||
| --EXPECT-- | ||
| Testing executeBulkWrite() | ||
| insert command reply includes operationTime: yes | ||
| Session reports last seen operationTime: yes | ||
|
|
||
| Testing executeCommand() | ||
| ping command reply includes operationTime: yes | ||
| Session reports last seen operationTime: yes | ||
|
|
||
| Testing executeQuery() | ||
| find command reply includes operationTime: yes | ||
| Session reports last seen operationTime: yes | ||
| ===DONE=== |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncomfortable with calling methods through the PHP/Zend layer here. Is there no other way to obtain the timestamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can certainly add logic to check for a Timestamp object and access these properties directly without invoking methods, and will do so.
That said, there is no other solution if we're dealing with a non-Timestamp TimestampInterface.
In my testing, if the
getTimestamp()throws,getIncrement()is never even called. This may be the result of logic insidezend_call_methodthat no-ops if a previous call failed or an exception has already been thrown. I still found that a bit odd, as here I am invokingzend_call_methodtwice without logic in-between. If you're more comfortable with it, I can also split up those calls and check for errors between them, instead of a combined check for undefinedztimestampandzincrementdown below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this goes through zend_execute, I expected that the throw would abort further calls. I think it's better to check for EG(exception) in between. But you're right on the non-Timestamp TimestampInterface. I now believe you shouldn't short-cut it as an exception for "Timestamp" too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
IS_UNDEFcheck was inspired by what I saw inzend_interfaces.c:I actually think the best approach is checking for
(Z_TYPE(retval) == IS_UNDEF || EG(exception)), as is done in the last example (zend_user_serialize).I personally don't see any harm in optimizing for the common use case (Timestamp instance), and simply forgot to make that change before I last updated this PR. I'll leave it out for now given your feedback, but we can always add it in later if you change your mind.