Finalize sharding support for 2.0 #1848
Conversation
4919549 to
e8f887e
Compare
| @@ -103,13 +103,13 @@ public static function failedToEnableSharding(string $dbName, string $errorMessa | |||
| )); | |||
| } | |||
|
|
|||
| public static function failedToEnsureDocumentSharding(string $className, string $errorMessage): self | |||
| public static function failedToEnsureDocumentSharding(string $className, string $errorMessage, ?\Throwable $previous = null): self | |||
There was a problem hiding this comment.
Is the previous exception ever used? I only see this called from SchemaManager::ensureDocumentSharding() later in this PR:
throw MongoDBException::failedToEnsureDocumentSharding($documentName, $e->getMessage());Does it make sense to type hint against the driver's RuntimeException here (or ServerException, in light of my comments to follow), or is that undesirable since this is technically public API? Alternatively, if this method isn't intended for public use you can simply annotate with @internal and do what you like.
| { | ||
| return new self(sprintf( | ||
| 'Failed to ensure sharding for document "%s". Error from MongoDB: %s', | ||
| $className, | ||
| $errorMessage | ||
| )); | ||
| ), 0, $previous); |
There was a problem hiding this comment.
I was going to ask if we should re-use the exception code from the driver's RuntimeException (in the event that it's significant), but I suppose users can always access that through the previous exception. I think this is fine as-is.
| @@ -599,7 +574,7 @@ public function enableShardingForDbByDocumentName(string $documentName): void | |||
| try { | |||
| $adminDb->command(['enableSharding' => $dbName]); | |||
| } catch (RuntimeException $e) { | |||
| if ($e->getCode() !== 23 || $e->getMessage() === 'already enabled') { | |||
| if ($e->getCode() !== 23 || strpos($e->getMessage(), 'already enabled') === false) { | |||
There was a problem hiding this comment.
Looking at the server error codes, 23 corresponds to "AlreadyInitialized". Two things:
- Consider making this an internal class constant (or private static) to avoid a magic literal here and provide some context.
- Technically, the driver only guarantees that an exception code originates from the server for instances of ServerException (introduced in 1.5). You likely still want to catch RuntimeException, since that considers other possible failures (e.g. socket errors), but error code checking should likely be restricted to ServerException.
| $class = $this->dm->getClassMetadata($documentName); | ||
|
|
||
| $database = $this->dm->getDocumentDatabase($documentName); | ||
| $collections = iterator_to_array($database->listCollections(['filter' => ['name' => $class->getCollection()]])); |
There was a problem hiding this comment.
Since Database::listCollections() returns a CollectionInfoIterator and you don't actually access the collection info here, would it make sense to use iterator_count() instead and bypass the array conversion?
| $document = new ShardedOne(); | ||
| $this->dm->persist($document); | ||
| $this->dm->flush(); | ||
|
|
||
| $class = ShardedOne::class; | ||
| $this->expectException(MongoDBException::class); | ||
| $this->expectExceptionMessageRegExp('#please create an index that starts with the (proposed )?shard key before sharding#i'); |
There was a problem hiding this comment.
Are we asserting a server error message here? If possible, I'd strongly suggest asserting that you receive a MongoDB\Driver\Exception\ServerException (1.5+) and use an error code instead. Newer versions of the server should be more consistent about error codes, while message strings can change at any time.
There was a problem hiding this comment.
I dropped checking against the MongoDB portion of the exception and only ensured that this is the exception we're throwing. I'd like to avoid exposing MongoDB error codes as our own and we're currently not exposing MongoDB exceptions in instances where we catch and handle them.
e8f887e to
5c9da67
Compare
5c9da67 to
8c429a0
Compare
8c429a0 to
51a3a12
Compare
51a3a12 to
d87a76d
Compare
Summary
This PR re-introduces full sharding support to ODM 2.0. Due to changes in the server, the implementation now requires the user to map an index covering the shard key. Previously, this index was created automatically by ODM based on an index suggestion returned by the server as a response to the
shardCollectioncommand. Since this is no longer returned, we can't implicitly create an index without duplicating the index check logic in the server. Thus, we require the user to create the index themselves.A compat layer for 1.3 should ideally warn the user about the change when an index is implicitly created when enabling sharding for a collection.