PHPC-1109: Support batchSize getMore option for command cursors #750

kvwalker · 2018-02-05T22:29:58Z

https://jira.mongodb.org/browse/PHPC-1109

jmikola · 2018-02-06T04:54:35Z

php_phongo.c

@@ -907,6 +907,10 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
 			bson_append_bool(&initial_reply, "tailable", -1, 1);
 		}

+		if (command->batch_size) {
+			bson_append_int32(&initial_reply, "batchSize", -1, command->batch_size);


On second thought, let's use bson_append_int64() here. The corresponding documentation in libmongoc (find_with_opts()) says "batchSize" and "maxAwaitTimeMS" are both "non-negative int64". Logic in Command.c already ensures they fall between 0 and UINT32_MAX (inclusive), so we don't have to worry about that. Assigning as an int64 will also ensure that UINT32_MAX will not be interpreted as a negative int64 down the line (an edge case, I know).

I'd also suggest changing the append function for "maxAwaitTimeMS" a few lines up in a second commit.

On that note, should we make the new batch_size member of the struct an uint64_t instead of an uint32_t below too then?

uint32_t should cast up to uint64_t without issue and we only store these values if they call within the unsigned 32-bit range. I think this is OK to remain as-is.

jmikola · 2018-02-06T04:57:08Z

src/MongoDB/Command.c

+
+		if (batch_size > UINT32_MAX) {
+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"batchSize\" option to be <= %" PRIu32 ", %" PRId64 " given", UINT32_MAX, batch_size);
+		}


IIRC, we discussed ignoring out-of-range values instead of throwing an exception, since this is internal behavior hidden from the user. Also, if they happen to use an invalid value, chances are the main command itself will yield an error. I'd suggest:

if (batch_size >= 0 && batch_size <= UINT32_MAX) { intern->batch_size = (uint32_t) batch_size; }

Note that if we were going to throw an exception here, it'd be important to return false after doing so. This all comes after the if (EG(exception)) return false; that trails BSON conversion, and it would therefore be possible for this php_phongo_command_init() function to return true so long as php_phongo_command_init_max_await_time_ms() didn't fail below.

Returning false on the first error will ensure that the calling function can act accordingly (technically not a problem in this case, since this function is the last statement of the Command constructor) and avoid cases where we end up throwing multiple exceptions (in that case, previous exceptions will get masked under the most recent and can only be accessed with Exception::getPrevious()).

jmikola · 2018-02-06T05:02:46Z

src/MongoDB/Command.c

@@ -69,6 +72,19 @@ static bool php_phongo_command_init(php_phongo_command_t *intern, zval *filter,
 		return false;
 	}

+	if (bson_iter_init (&iter, intern->bson) && bson_iter_find_descendant (&iter, "cursor.batchSize", &sub_iter) && BSON_ITER_HOLDS_INT(&sub_iter)) {


Spaces between the function name and open parenthesis is part of libmongoc's coding standard, but we don't follow it within the PHP extension. bson_iter_init(...) works fine.

jmikola · 2018-02-06T05:07:33Z

I didn't see a test, which might have been an oversight if you just forgot to add a new file. I whipped up the following test file and verified that this works as it should:

--TEST--
MongoDB\Driver\Command batchSize applies to getMore
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS('STANDALONE'); CLEANUP(STANDALONE); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

class Test implements MongoDB\Driver\Monitoring\CommandSubscriber
{
    public function executeCommand()
    {
        $manager = new MongoDB\Driver\Manager(STANDALONE);

        $bulk = new MongoDB\Driver\BulkWrite;
        $bulk->insert(['_id' => 1]);
        $bulk->insert(['_id' => 2]);
        $bulk->insert(['_id' => 3]);
        $bulk->insert(['_id' => 4]);
        $bulk->insert(['_id' => 5]);

        $manager->executeBulkWrite(NS, $bulk);

        MongoDB\Driver\Monitoring\addSubscriber($this);

        $command = new MongoDB\Driver\Command([
            'aggregate' => COLLECTION_NAME,
            'pipeline' => [],
            'cursor' => ['batchSize' => 2],
        ]);

        $cursor = $manager->executeCommand(DATABASE_NAME, $command);

        $cursor->toArray();

        MongoDB\Driver\Monitoring\removeSubscriber($this);
    }

    public function commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event)
    {
        $command = $event->getCommand();

        if ($event->getCommandName() === 'aggregate') {
            printf("aggregate command specifies batchSize: %d\n", $command->cursor->batchSize);
        }

        if ($event->getCommandName() === 'getMore') {
            printf("getMore command specifies batchSize: %d\n", $command->batchSize);
        }
    }

    public function commandSucceeded(MongoDB\Driver\Monitoring\CommandSucceededEvent $event)
    {
        $reply = $event->getReply();

        if ($event->getCommandName() === 'aggregate') {
            printf("aggregate response contains %d document(s)\n", count($reply->cursor->firstBatch));
        }

        if ($event->getCommandName() === 'getMore') {
            printf("getMore response contains %d document(s)\n", count($reply->cursor->nextBatch));
        }
    }

    public function commandFailed(MongoDB\Driver\Monitoring\CommandFailedEvent $event)
    {
    }
}

(new Test)->executeCommand();

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
aggregate command specifies batchSize: 2
aggregate response contains 2 document(s)
getMore command specifies batchSize: 2
getMore response contains 2 document(s)
getMore command specifies batchSize: 2
getMore response contains 1 document(s)
===DONE===

I might suggest adding one more test to demonstrate that a batchSize of 0 is ignored and is omitted from the getMore command. In that case, in the context of this test, the first getMore response should contain three documents and there shouldn't be a second getMore at all.

kvwalker · 2018-02-06T15:41:54Z

tests/command/cursor-batchsize-002.phpt

+aggregate command specifies batchSize: 0
+aggregate response contains 0 document(s)
+getMore command specifies batchSize: no
+getMore response contains 5 document(s)


According to the manual, a batchSize of 0 means an empty first batch and is useful for quickly returning a cursor or failure message without doing significant server-side work so the aggregate should return 0 documents and the getMore should return the remaining documents (5 in this case).

Ah, that's right. I was only thinking about the two getMore requests in my last comment.

Side note: this is related to PHPLIB-312.

jmikola · 2018-02-06T15:57:43Z

tests/command/cursor-batchsize-001.phpt

+
+        $manager = new MongoDB\Driver\Manager(STANDALONE);
+
+		$bulkWrite = new MongoDB\Driver\BulkWrite;


Indentation looks off here. PHP source uses four-space indents (not tabs as in C sources). I realize that's confusing :)

jmikola · 2018-02-06T15:58:19Z

tests/command/cursor-batchsize-001.phpt

+		$writeResult = $manager->executeBulkWrite(NS, $bulkWrite);
+		printf("Inserted: %d\n", $writeResult->getInsertedCount());
+
+		$command = new MongoDB\Driver\Command(array(


We can use short array syntax (e.g. [ ]) whenever possible.

jmikola · 2018-02-06T15:59:06Z

tests/command/cursor-batchsize-002.phpt

@@ -0,0 +1,84 @@
+--TEST--
+MongoDB\Driver\Command batchSize of zero ignored


"batchSize of zero is ignored for getMore"

jmikola · 2018-02-06T15:59:37Z

tests/command/cursor-batchsize-001.phpt

@@ -0,0 +1,86 @@
+--TEST--
+MongoDB\Driver\Command batchSize applies to getMore


"non-zero batchSize applies to getMore" (balances out the other test name)

jmikola · 2018-02-06T16:00:07Z

tests/command/cursor-batchsize-002.phpt

+
+        $manager = new MongoDB\Driver\Manager(STANDALONE);
+
+		$bulkWrite = new MongoDB\Driver\BulkWrite;


Indentation here as well.

jmikola · 2018-02-06T16:00:25Z

tests/command/cursor-batchsize-002.phpt

+MongoDB\Driver\Command batchSize of zero ignored
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php NEEDS_CRYPTO(); ?>


Looks like copypasta from the causal consistency tests and can be removed.

jmikola · 2018-02-06T16:00:34Z

tests/command/cursor-batchsize-001.phpt

+MongoDB\Driver\Command batchSize applies to getMore
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php NEEDS_CRYPTO(); ?>


Can remove.

jmikola

I changed the target branch to v1.4. We can rebase these commits together before merging this down.

kvwalker requested a review from jmikola February 5, 2018 22:30

jmikola reviewed Feb 6, 2018

View reviewed changes

kvwalker commented Feb 6, 2018

View reviewed changes

jmikola reviewed Feb 6, 2018

View reviewed changes

jmikola changed the base branch from master to v1.4 February 6, 2018 17:03

jmikola approved these changes Feb 6, 2018

View reviewed changes

kvwalker added 2 commits February 6, 2018 13:12

PHPC-1109: Support batchSize getMore option for command cursors

bc5ce89

Append maxAwaitTimeMS as int64 instead of int32

2cdc10f

kvwalker force-pushed the PHPC-1109 branch from 94f5c9d to 2cdc10f Compare February 6, 2018 18:13

kvwalker merged commit 2cdc10f into mongodb:v1.4 Feb 6, 2018

kvwalker added a commit that referenced this pull request Feb 6, 2018

Merge pull request #750

876492a

kvwalker deleted the PHPC-1109 branch February 6, 2018 18:17


		$manager = new MongoDB\Driver\Manager(STANDALONE);

		$bulkWrite = new MongoDB\Driver\BulkWrite;

		@@ -0,0 +1,84 @@
		--TEST--
		MongoDB\Driver\Command batchSize of zero ignored

		@@ -0,0 +1,86 @@
		--TEST--
		MongoDB\Driver\Command batchSize applies to getMore

PHPC-1109: Support batchSize getMore option for command cursors #750

PHPC-1109: Support batchSize getMore option for command cursors #750

Uh oh!

Conversation

kvwalker commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!