Riak #6

Merged
merged 6 commits into from Jul 3, 2012

Projects

None yet

3 participants

@Baachi
Contributor
Baachi commented Jun 19, 2012

I added the riak storage.

It use the update from the offical php-driver.

@stof stof commented on an outdated diff Jun 19, 2012
lib/Doctrine/KeyValueStore/Storage/RiakStorage.php
+ *
+ * @param \Riak\Client $riak
+ * @param string $bucketName
+ */
+ public function __construct(Client $riak, $bucketName)
+ {
+ $this->client = $riak;
+ $this->bucketName = $bucketName;
+ }
+
+ /**
+ * Determine if the storage supports updating only a subset of properties,
+ * or if all properties have to be set, even if only a subset of properties
+ * changed.
+ *
+ * @return bool
@stof
stof Jun 19, 2012 Member

you should use {@inheritDoc} for all methods documented in the interface, to avoid duplicating all phpdoc. It makes it easier to maintain it (typos occurs only in 1 place for instance)

@stof stof commented on an outdated diff Jun 19, 2012
...trine/Tests/KeyValueStore/Storage/RiakStorageTest.php
+{
+ /**
+ * @var RiakStorage
+ */
+ private $storage;
+
+ /**
+ * @var \PHPUnit_Framework_MockObject_MockObject
+ */
+ private $riak;
+
+ protected function setup()
+ {
+ $this->riak = $this->getMockBuilder('Riak\\Client')
+ ->disableOriginalConstructor()
+ ->getMock();
@stof
stof Jun 19, 2012 Member

please use 4 spaces per level for the indentation (you did it in some places in this file but not everywhere)

@beberlei beberlei and 1 other commented on an outdated diff Jun 20, 2012
composer.json
@@ -3,6 +3,9 @@
"require": {
"doctrine/common": "*"
},
+ "require-dev": {
@beberlei
beberlei Jun 20, 2012 Member

You should add riak to the "suggest" key aswell, what do you think?

@Baachi
Baachi Jun 20, 2012 Contributor

Yes, it makes sense. I will add riak to the suggest section.

@beberlei beberlei and 1 other commented on an outdated diff Jun 20, 2012
lib/Doctrine/KeyValueStore/Storage/RiakStorage.php
+ * @var \Riak\Client
+ */
+ protected $client;
+
+ /**
+ * @var string
+ */
+ protected $bucketName;
+
+ /**
+ * Constructor
+ *
+ * @param \Riak\Client $riak
+ * @param string $bucketName
+ */
+ public function __construct(Client $riak, $bucketName)
@beberlei
beberlei Jun 20, 2012 Member

The bucket name should be removed and $storageName from the different methods be used or not? To namespace the different objects into different storages.

@Baachi
Baachi Jun 20, 2012 Contributor

I don't know whether it makes sense.
If i would use $storageName for the bucket. It will create for each object a new bucket.
I think it's not fast enough

I will make a benchmark with :

  • one bucket for all objects
  • one bucket per object
@Baachi
Baachi Jun 20, 2012 Contributor

Okay i checked this and you are right.
It's better to remove $bucketName from the constructor and use $storageName, because riak
can better scale more small buckets as than one big bucket.

@Baachi
Contributor
Baachi commented Jun 20, 2012

I've made some changes according @stof suggestions.

@stof stof and 1 other commented on an outdated diff Jun 20, 2012
composer.json
@@ -3,6 +3,12 @@
"require": {
"doctrine/common": "*"
},
+ "suggest": {
+ "riak/riak-client": "dev-master"
@stof
stof Jun 20, 2012 Member

suggest does not expect a version constraint but a string displayed to the user. so you could make it better by using "riak/riak-client": "to use the Riak storage". This will output "doctrine/key-value-store" suggest installing "riak/riak-client" (to use the Riak storage)

@Baachi
Baachi Jun 20, 2012 Contributor

Ah awesome!
Thank you ;)

@Baachi
Contributor
Baachi commented Jul 3, 2012

@beberlei It's mergeable?

@beberlei beberlei merged commit 801400d into doctrine:master Jul 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment