PHPLIB-476: Consider transaction readPreference in select_server #1178
Conversation
| } | ||
|
|
||
| // Manager::selectServer() defaults to a primary read preference |
|
|
||
| $this->assertTrue($session->isInTransaction()); | ||
| $this->assertInstanceOf(Server::class, $session->getServer(), 'Session is pinned'); | ||
| $this->assertEquals($session->getServer(), select_server($client->getManager(), ['session' => $session])); |
There was a problem hiding this comment.
Note: this test is still a bit dubious, as we don't have any insight into what Manager::selectServer() is actually doing. That may change with server selection logging (PHPLIB-1000).
Currently, there's a chance that server selection just happens to select the same server that was pinned (see: Topology type: Sharded in the Server Selection spec). Supplying an alternative read preference in the operation-level options doesn't really affect this, since it wouldn't influence mongos selection. Again, logging would help verify that.
Having said all that, I think adding a functional test is a good first step since we previously had no direct test coverage for this function. This file at least provides a starting point to add more coverage down the line. I've left a comment in PHPLIB-1000 to that effect.
jmikola
force-pushed
the
phplib-476
branch
2 times, most recently
from
0459bc2 to
7262c64
Compare
This also refactors the conditionals in extract_session_from_options and extract_read_preference_from_options to improve readability. select_server() previously did not consider the read preference of an active transaction. This isn't very significant, as transactions require a primary read preference, but it is correct to do so.
| * | ||
| * @internal | ||
| */ | ||
| function select_server_for_write(Manager $manager, array $options): Server |
There was a problem hiding this comment.
Quoting Transactions: readPreference:
The read preference to use for all read operations in this transaction.
The select_server_for_write() method here allows us to avoid considering a transaction's readPreference option for a known write command. I intentionally chose not to use this method for read methods that are typically run on primaries (e.g. collection and database enumeration). And it's not used for Database::command() sinec that's considered a read operation (see: runCommand spec).
It's clear we don't have comprehensive test coverage for this across all operations (for example, there's no test that tries to list databases within a transaction), but I'm glad that I was alerted to this oversight in some failing legacy transaction spec tests.
|
Test failures seem to be due to an unrelated change in drivers-evergreen-tools, which I'm tracking in https://github.com/mongodb-labs/drivers-evergreen-tools/pull/352/files#r1342969157. I'll wait for that to get resolved before moving forward with this PR. |
| @@ -418,7 +418,7 @@ public function createSearchIndex($definition, array $options = []): string | |||
| public function createSearchIndexes(array $indexes, array $options = []): array | |||
| { | |||
| $operation = new CreateSearchIndexes($this->databaseName, $this->collectionName, $indexes, $options); | |||
| $server = select_server($this->manager, $options); | |||
| $server = select_server_for_write($this->manager, $options); | |||
There was a problem hiding this comment.
I don't know if that's relevant for search indexes.
These commands internally proxy the search index management commands to a separate process that runs alongside an Atlas cluster. As such, read concern and write concern are not relevant for the search index management commands.
https://github.com/mongodb/specifications/blob/db3114e957f7c0976a1af09882dbb46cb4a70049/source/index-management/index-management.rst#where-are-read-concern-and-write-concern
There was a problem hiding this comment.
I'm familiar with mongot, but does that mean mongod has absolutely not state when it comes to search indexes? Perhaps that's the case, as listSearchIndexes() is distinct from listIndexes().
This also raises the question of what happens if the search index operations are executed within a transaction. Presumably they don't maintain state on mongod at all (and thus operate outside of a transaction), but it's not clear to me if mongod would actually restrict their usage during an active transaction.
That said, I don't see the harm in requiring a primary in this case since it is a write operation from the user's perspective.
https://jira.mongodb.org/browse/PHPLIB-476
This also refactors the conditionals in extract_session_from_options and extract_read_preference_from_options to improve readability.
select_server() previously did not consider the read preference of an active transaction. This isn't very significant, as transactions require a primary read preference, but it is correct to do so.