Skip to content

[WIP] Feature sharding #325

Closed
wants to merge 6 commits into from

8 participants

@tecbot
tecbot commented Jun 11, 2012

Hi,

i have implemented a first version of sharding support. I added a new annotation "QueryFields" to change the core query routine for a document.

The next step is to update the reference mappings to include the query fields in the reference object. But first I want some feedback from you.

Issue: #304

Regards
Thomas

@jwage jwage and 1 other commented on an outdated diff Jun 11, 2012
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -741,6 +738,40 @@ private function loadReferenceManyWithRepositoryMethod(PersistentCollection $col
}
/**
+ * Gets the query for the document.
+ *
+ * @param mixed $document
+ *
+ * @return array $query
+ */
+ public function getDocumentQuery($document)
+ {
+ if($this->class->hasQueryFields()) {
@jwage
Doctrine member
jwage added a note Jun 11, 2012

All the other code in Doctrine has a space between if and (

@tecbot
tecbot added a note Jun 11, 2012

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jwage
Doctrine member
jwage commented Jun 11, 2012

Looks interesting. So the queryFields will be stored in the reference document? What else do you see as being needed to support sharding fully?

@tecbot
tecbot commented Jun 11, 2012

Yes we need to add the query fields to the reference document to make a correct shard query and after this we support sharding fully so we need nothing more.

@jwage
Doctrine member
jwage commented Jun 11, 2012

Ok. Can't wait to see the rest.

@tecbot
tecbot commented Jun 11, 2012

Ok, I will finish it tomorrow :)

@balboah
balboah commented Jun 13, 2012

Did you? :)

@jmikola
Doctrine member
jmikola commented Jun 13, 2012

I'll plan to take a look at this tomorrow on my local machine with a small sharding setup. @tecbot: Please let me know if anything is still TODO.

@tecbot
tecbot commented Jun 14, 2012

@jmikola currently i'm working on the db ref stuff for reference mapping.

@henrikbjorn henrikbjorn and 1 other commented on an outdated diff Jun 14, 2012
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
+ *
+ * @return array $fields The fields for the query.
+ */
+ public function getQueryFields()
+ {
+ return $this->queryFields;
+ }
+
+ /**
+ * Checks whether this document has custom query fields or not.
+ *
+ * @return boolean
+ */
+ public function hasQueryFields()
+ {
+ return $this->queryFields ? true : false;
@henrikbjorn
henrikbjorn added a note Jun 14, 2012

couldnt this be return (boolean) $this->queryFields ?

@tecbot
tecbot added a note Jun 14, 2012

fixed in the next commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@henrikbjorn henrikbjorn and 2 others commented on an outdated diff Jun 14, 2012
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
+ {
+ return $this->queryFields ? true : false;
+ }
+
+ /**
+ * Sets the custom query fields for this Document.
+ *
+ * @param array $fields Array of fields for the query.
+ */
+ public function setQueryFields($fields)
+ {
+ if(!is_array($fields)) {
+ $fields = array($fields);
+ }
+
+ $this->queryFields = $fields;
@henrikbjorn
henrikbjorn added a note Jun 14, 2012

$this->queryFields = is_array($fields) ? $fields : array($fields);

Imo easier to read.

@tecbot
tecbot added a note Jun 14, 2012

fixed in the next commits

@omgnull
omgnull added a note Aug 3, 2012

Yes easier, but using if () provide better performance, what is more important?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tecbot
tecbot commented Jun 15, 2012

@jmikola, @jwage : I pushed the initial DBref logic for sharding also I implemented lazy loading for referenece one inverse side. Currently shard keys for embedded or reference documents are not supported.

@jmikola
Doctrine member
jmikola commented Jun 19, 2012

@tecbot: I can't find contact info for you, but could we sync up in IRC or Google chat later today to discuss the PR? I'm in the #doctrine-dev channel.

@tecbot
tecbot commented Jun 19, 2012

@jmikola ok. I'm in the the #doctrine-dev channel now.

@balboah
balboah commented Jul 9, 2012

What's the progress on this feature?

@tecbot
tecbot commented Jul 9, 2012

I want to finish the feature end of this week.

@balboah
balboah commented Jul 9, 2012

What is left to be done? Can I help in some way?

@tecbot
tecbot commented Jul 9, 2012
  • rename QueryFields to ShardedFields
  • throw exceptions if user want to change sharded fields
  • add the possibility to set sharded fields for embedded and reference documents
  • code cleanup and more unit testing

if you want you can help :), i haven't started any task of this at the moment.

@jmikola jmikola commented on the diff Jul 9, 2012
tests/Doctrine/ODM/MongoDB/Tests/Query/ExprTest.php
@@ -148,7 +148,7 @@ public function testReferencesUsesMinimalKeys()
$dm->expects($this->once())
->method('createDBRef')
- ->will($this->returnValue(array('$ref' => 'coll', '$id' => '1234', '$db' => 'db')));
+ ->will($this->returnValue(array('$id' => '1234')));
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

Why is $ref missing here? I can understand removing $db, as it's optional, but reference objects require $ref and $id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
...M/MongoDB/Mapping/Annotations/DoctrineAnnotations.php
@@ -61,6 +61,8 @@
final class DiscriminatorMap extends Annotation {}
/** @Annotation */
final class DiscriminatorValue extends Annotation {}
+/** @Annotation */
+final class QueryFields extends Annotation {}
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

I think it would make more sense for this to be named ShardKeys, unless there is some alternative use case I'm missing. Additionally, we can consider mapping one or more fields with @ShardKey down the line. Specifying an array of MongoDB field names (they're not property names, right?) as a class-level annotation works for now, though.

@tecbot
tecbot added a note Jul 9, 2012

Ok. I will change this to @ShardKey

@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

The class-level annotation should be @ShardKeys, since users may specify one or more fields. The field-level annotation can be @ShardKey later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -183,6 +183,11 @@ class ClassMetadataInfo implements \Doctrine\Common\Persistence\Mapping\ClassMet
public $requireIndexes = false;
/**
+ * READ-ONLY: The fields for the query for the document collection.
+ */
+ public $queryFields = array();
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

Naming this shardKeys would also help readability (same point as my comment about the annotation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -240,13 +240,17 @@ public function executeInserts(array $options = array())
$upsertOptions = $options;
$upsertOptions['upsert'] = true;
foreach ($upserts as $oid => $data) {
- $criteria = array('_id' => $data[$this->cmd.'set']['_id']);
- unset($data[$this->cmd.'set']['_id']);
+ $query = $this->getDocumentQuery($this->queuedInserts[$oid]);
+ foreach ($query as $field => $value) {
+ if (isset($data[$this->cmd.'set'][$field])) {
+ unset($data[$this->cmd.'set'][$field]);
+ }
+ }
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

I assume this is for ensuring that shard keys aren't modified, so you'll end up replacing this with an exception?

Can we simply read the shard key fields from the class metadata here? Calling getDocumentQuery() seems like unnecessary overhead just to get some field names.

@tecbot
tecbot added a note Jul 9, 2012

yes in the next update it throws exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
* @param object $document The document to refresh.
*/
- public function refresh($id, $document)
+ public function refresh($document)
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

Does the removal of the first argument signify a BC break, or would this public method never be called by the user. I assume perhaps it's further down the line from DocumentManager::refresh(), so users wouldn't call this.

@tecbot
tecbot added a note Jul 9, 2012

IMO its ok. It was used only one time in the core. I don't thinks users used it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -449,8 +448,8 @@ private function wrapCursor(BaseCursor $cursor)
*/
public function exists($document)
{
- $id = $this->class->getIdentifierObject($document);
- return (bool) $this->collection->findOne(array(array('_id' => $id)), array('_id'));
+ $query = $this->getDocumentQuery($document);
+ return (bool) $this->collection->findOne($query, array('_id'));
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

Shouldn't the second argument be array('_id' => 1)? I realize that's not your fault, but this would be a good place to correct it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
...octrine/ODM/MongoDB/Tests/Functional/ShardingTest.php
+
+ $shardRef = $this->dm->createQueryBuilder('Doctrine\ODM\MongoDB\Tests\Functional\ShardRefDocument')
+ ->field('name')->equals('foo')
+ ->hydrate(false)
+ ->getQuery()
+ ->getSingleResult();
+
+ $this->assertTrue(isset($shardRef['embeddedShardReference']['$embedOne.sk']));
+ $this->assertEquals($shardKey, $shardRef['embeddedShardReference']['$embedOne.sk']);
+ }
+}
+
+/**
+ * @ODM\Document
+ */
+class ShardRefDocument
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

I realize this class is for testing references to sharded documents, but should we also test the ability to use something like simpleShardReference.$id as a shard key?

@tecbot
tecbot added a note Jul 9, 2012

this is a open task to support shard keys for referencens and embedded documents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola commented on the diff Jul 9, 2012
lib/Doctrine/ODM/MongoDB/DocumentManager.php
* @return mixed|object The document reference.
*/
- public function getReference($documentName, $identifier)
+ public function getReference($documentName, $identifier, $query = null, array $sort = null)
@jmikola
Doctrine member
jmikola added a note Jul 9, 2012

What is the $sort parameter used for, if this is for creating a proxy object?

@tecbot
tecbot added a note Jul 9, 2012

yes its for the inverse side proxy. in this case we need the $sort parameter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Lusitanian

@jmikola Ping, just curious on what the progress of this is/if it's going to be implemented in 1.0

@alcaeus
Doctrine member
alcaeus commented Mar 14, 2016

Superseded by #1385.

@alcaeus alcaeus closed this Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.