PHPLIB-276: Add maxAwaitTimeMS support for change streams #468
Conversation
src/Operation/Aggregate.php
Outdated
| $cmdOptions['maxAwaitTimeMS'] = $this->options['maxAwaitTimeMS']; | ||
| } | ||
|
|
||
| if (isset($this->options['maxTimeMS'])) { |
There was a problem hiding this comment.
Is this necessary? I think it's already covered by foreach (['comment', 'maxTimeMS'] as $option) about 15 lines prior.
| /* Make sure we await results for at least maxAwaitTimeMS, since no new | ||
| * documents should be inserted to wake up the server's command thread. | ||
| * Also ensure that we don't wait too long (server default is one | ||
| * second). */ |
There was a problem hiding this comment.
Can you add a space in this comment to align the asterisks?
| /* Make sure we await results for at least maxAwaitTimeMS, since no new | ||
| * documents should be inserted to wake up the server's query thread. | ||
| * Also ensure that we don't wait too long (server default is one | ||
| * second). */ |
There was a problem hiding this comment.
Extra space to align asterisks here, too.
| $this->collection = new Collection($this->manager, $this->getDatabaseName(), $this->getCollectionName()); | ||
| $maxAwaitTimeMS = 10; | ||
| $changeStreamResult = $this->collection->watch([], ['maxAwaitTimeMS' => $maxAwaitTimeMS]); | ||
| $changeStreamResult->next(); |
There was a problem hiding this comment.
I know you removed the rewind() call in the constructor, but can you confirm whether $changeStreamResult->rewind() invokes getMore and introduces a delay?
If so, we can do the assert and time check on that initial rewind(). It would also set a good example for anyone looking at the test files for guidance (ideally, users should rewind first).
|
Travis appears to have failed because the jobs are using If this was the first PR requiring 1.4.0, you'd need to bump the |
kvwalker
force-pushed
the
PHPLIB-276-1
branch
from
4d05d51 to
3d353f1
Compare
src/Operation/Aggregate.php
Outdated
|
|
||
| if (isset($this->options['writeConcern'])) { | ||
| $cmd['writeConcern'] = \MongoDB\write_concern_as_document($this->options['writeConcern']); | ||
| } |
There was a problem hiding this comment.
Assuming this commit was rebased, readConcern and writeConcern were moved in 091d8bd#diff-3b2e2c1a4b1425910f13159568854106L302 to a separate createOptions() method. If this commit is adding them back, it's likely that these got put back when selecting the maxAwaitTimeMS addition (git likely presented them all as one addition). This would have required splitting the diff during the rebase, which I'm happy to walk through and demonstrate.
kvwalker
force-pushed
the
PHPLIB-276-1
branch
from
3d353f1 to
e83baad
Compare
jmikola
changed the title
|
|
||
| $iterator->next(); | ||
| } | ||
| } |
This inserts an additional document so that we can assert that the first rewind and next calls do not block, while the last next does issue a getMore and block.
kvwalker
force-pushed
the
PHPLIB-276-1
branch
from
69cfb9f to
bff5e2e
Compare
2 participants
https://jira.mongodb.org/browse/PHPLIB-276