Sharding #162

Merged
merged 10 commits into from Jun 26, 2012

Conversation

Projects
None yet
3 participants
@beberlei
Owner

beberlei commented Jun 16, 2012

This moves the Doctrine shards project into DBAL. It is self-contained and doe not affect the normal DBAL code. It does however add an interesting new concept for changing databases: SchemaSynchronizer. It is essentially an Interface for SchemaTool operations in ORM. This allows sharding implementations to synchronize multiple databases at once.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 16, 2012

This pull request fails (merged 0b4251e2 into 2cac730).

This pull request fails (merged 0b4251e2 into 2cac730).

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

The content of the docs folder should be moved to the DBAL documentation instead

Member

stof commented Jun 16, 2012

The content of the docs folder should be moved to the DBAL documentation instead

@stof

View changes

lib/Doctrine/DBAL/Schema/Synchronizer/SingleDatabaseSynchronizer.php
+
+ if ($noDrops) {
+ return $schemaDiff->toSaveSql($this->platform);
+ } else {

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

no need to use else as the if returns

@stof

stof Jun 16, 2012

Member

no need to use else as the if returns

@stof

View changes

lib/Doctrine/DBAL/Schema/Synchronizer/SingleDatabaseSynchronizer.php
+ public function getDropAllSchema()
+ {
+ $sm = $this->conn->getSchemaManager();
+ $visitor = new \Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector($this->platform);

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

you should add a use statement for this class

@stof

stof Jun 16, 2012

Member

you should add a use statement for this class

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

actually, the use statement is even already there :)

@stof

stof Jun 16, 2012

Member

actually, the use statement is even already there :)

@stof

View changes

lib/Doctrine/DBAL/Sharding/PoolingShardConnection.php
+ public function close()
+ {
+ unset($this->_conn);
+ unset($this->activeConnections);

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

unset removes the properties from the class, which can lead to E_STRICT errors later if something tries to read them (as it will try to read undefined properties), which is likely to happen if you use 2 instance of the class. You should set the properties to null instead.

@stof

stof Jun 16, 2012

Member

unset removes the properties from the class, which can lead to E_STRICT errors later if something tries to read them (as it will try to read undefined properties), which is likely to happen if you use 2 instance of the class. You should set the properties to null instead.

@stof

View changes

lib/Doctrine/DBAL/Sharding/PoolingShardConnection.php
+ public function isConnected($shardId = null)
+ {
+ if ($shardId === null) {
+ return ($this->_conn !== null);

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

no need to use the braces around the condition

@stof

stof Jun 16, 2012

Member

no need to use the braces around the condition

@stof

View changes

lib/Doctrine/DBAL/Sharding/PoolingShardConnection.php
+ private $connections;
+
+ /**
+ * @var PoolingShardManager

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

the phpdoc should mention the interface, not the implementation

@stof

stof Jun 16, 2012

Member

the phpdoc should mention the interface, not the implementation

@stof

View changes

lib/Doctrine/DBAL/Sharding/SQLAzure/SQLAzureFederationsSynchronizer.php
+ * SQL Azure Schema Synchronizer
+ *
+ * Will iterate over all shards when performing schema operations. This is done
+ * by partioning the passed schema into subschemas for the federation and the

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

typo here

@stof

stof Jun 16, 2012

Member

typo here

+ $this->synchronizer = $sync ?: new SingleDatabaseSynchronizer($conn);
+ }
+
+

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

extra empty line here

@stof

stof Jun 16, 2012

Member

extra empty line here

@stof

View changes

lib/Doctrine/DBAL/Sharding/SQLAzure/SQLAzureFederationsSynchronizer.php
+ private $synchronizer;
+
+ const FEDERATION_TABLE_FEDERATED = 'azure.federated';
+ const FEDERATION_DISTRIBUTION_NAME = 'azure.federatedOnDistributionName';

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

constants should be defined before properties

@stof

stof Jun 16, 2012

Member

constants should be defined before properties

@stof

View changes

lib/Doctrine/DBAL/Sharding/SQLAzure/SQLAzureFederationsSynchronizer.php
+ foreach ($sql as $s) {
+ $this->conn->exec($s);
+ }
+ }

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

you should have an abstract synchronizer as you are duplicating some methods in the different implementations

@stof

stof Jun 16, 2012

Member

you should have an abstract synchronizer as you are duplicating some methods in the different implementations

@stof

View changes

lib/Doctrine/DBAL/Sharding/SQLAzure/SQLAzureShardManager.php
+ }
+
+ /**
+ * @override

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

you should remove @override (especially as you are not overriding anything)

@stof

stof Jun 16, 2012

Member

you should remove @override (especially as you are not overriding anything)

+ */
+ public function acceptSequence(Sequence $sequence)
+ {
+ }

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

should we add an AbstractVisitor implementing dummy methods for the interface so that you only defines the method you really need in child classes (similar to Twig extensions or Symfony form types) ?

@stof

stof Jun 16, 2012

Member

should we add an AbstractVisitor implementing dummy methods for the interface so that you only defines the method you really need in child classes (similar to Twig extensions or Symfony form types) ?

@stof

View changes

docs/examples/sharding/bootstrap.php
+use Doctrine\DBAL\DriverManager;
+use Doctrine\Shards\DBAL\SQLAzure\SQLAzureShardManager;
+
+require_once "vendor/composer/autoload.php";

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

the path is wrong

@stof

stof Jun 16, 2012

Member

the path is wrong

@stof

View changes

lib/Doctrine/DBAL/Sharding/PoolingShardConnection.php
+ $params = $this->getParams();
+ $this->shardManager = new PoolingShardManager($this, $params['shardChoser']);
+ }
+ return $this->shardManager;

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 16, 2012

Member

I find this method weird. I don't think the connection should be responsible to create the ShardManager (especially as the connection is inside the shard manager). Btw, for the SQLAzure implementation, you create the shard manager directly by passing it a connection instead of using a getter on the connection (which does not exist)

@stof

stof Jun 16, 2012

Member

I find this method weird. I don't think the connection should be responsible to create the ShardManager (especially as the connection is inside the shard manager). Btw, for the SQLAzure implementation, you create the shard manager directly by passing it a connection instead of using a getter on the connection (which does not exist)

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jun 25, 2012

Owner

This is necessary, because the Connection has control over all the configuration parameters.

@beberlei

beberlei Jun 25, 2012

Owner

This is necessary, because the Connection has control over all the configuration parameters.

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 25, 2012

Member

I don't see how this is necessary. Look at the SQLAzure implementation: it does not use the Connection as a factory

@stof

stof Jun 25, 2012

Member

I don't see how this is necessary. Look at the SQLAzure implementation: it does not use the Connection as a factory

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 25, 2012

This pull request fails (merged 672dc1cf into 2cac730).

This pull request fails (merged 672dc1cf into 2cac730).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 25, 2012

This pull request fails (merged 33a7c6b2 into 2cac730).

This pull request fails (merged 33a7c6b2 into 2cac730).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 26, 2012

This pull request fails (merged 12f381f into baf30ae).

This pull request fails (merged 12f381f into baf30ae).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 26, 2012

This pull request passes (merged 0dd4b6a into baf30ae).

This pull request passes (merged 0dd4b6a into baf30ae).

beberlei added a commit that referenced this pull request Jun 26, 2012

@beberlei beberlei merged commit dd16703 into master Jun 26, 2012

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 26, 2012

This pull request fails (merged b6f48e1 into baf30ae).

This pull request fails (merged b6f48e1 into baf30ae).

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