Problem/Motivation

Redis Sorted Sets are, similarly to Redis Sets, non repeating collections of Strings. The difference is that every member of a Sorted Set is associated with score, that is used in order to take the sorted set ordered, from the smallest to the greatest score. While members are unique, scores may be repeated.

https://redis.io/topics/data-types#sorted-sets

Borrowing the idea from Redis, Drupal's key value store should introduce sorted sets.

The current need for this comes via the workflow initiative where we would like to store data in a key/value store but want to be able to order them or do range queries. We'd call it a sequence index and store entity activity, so that when we do content replication we can replicate in the order that activity took place.

Proposed resolution

We already have this in contrib (http://www.drupal.org/project/key_value) and hope it's just a simple case of moving to core.

Remaining tasks

Write a patch

User interface changes

none

API changes

An additional key value store

Data model changes

An extra database table for the new store

CommentFileSizeAuthor
#30 interdiff.txt708 bytestimmillwood
#30 2831428-30.patch17.83 KBtimmillwood
#28 interdiff.txt812 bytestimmillwood
#28 2831428-28.patch17.83 KBtimmillwood
#23 interdiff.txt6.55 KBtimmillwood
#23 2831428-23.patch17.78 KBtimmillwood
#19 interdiff.txt3.72 KBtimmillwood
#19 2831428-19.patch17.95 KBtimmillwood
#17 interdiff.txt14.48 KBtimmillwood
#17 2831428-17.patch17.95 KBtimmillwood
#15 interdiff.txt8.6 KBtimmillwood
#15 2831428-15.patch17.16 KBtimmillwood
#13 interdiff.txt2.47 KBtimmillwood
#13 2831428-13.patch16.02 KBtimmillwood
#11 interdiff.txt1.43 KBtimmillwood
#11 2831428-11.patch15.97 KBtimmillwood
#9 interdiff.txt6.36 KBtimmillwood
#9 2831428-9.patch14.73 KBtimmillwood
#7 interdiff.txt5.4 KBtimmillwood
#7 2831428-7.patch12.8 KBtimmillwood
#6 interdiff.txt2.48 KBtimmillwood
#6 2831428-6.patch12.82 KBtimmillwood
#5 interdiff.txt5.67 KBtimmillwood
#5 2831428-5.patch13.14 KBtimmillwood
#4 2831428-4.patch14.77 KBtimmillwood
#2 2831428-2.patch15.21 KBtimmillwood
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Active » Needs review
FileSize
15.21 KB

Initial patch

Basically a port direct from contrib, I think we can drop the base classes.

Status: Needs review » Needs work

The last submitted patch, 2: 2831428-2.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
14.77 KB
timmillwood’s picture

FileSize
13.14 KB
5.67 KB

Removing the base classes and interfaces.

timmillwood’s picture

FileSize
12.82 KB
2.48 KB

Tidying up.

Think this is ready for a wider review.

timmillwood’s picture

FileSize
12.8 KB
5.4 KB

Removing deprecated test methods.

amateescu’s picture

Status: Needs review » Needs work

The patch looks really straightforward, but most of the classes/properties/methods are missing documentation. Especially KeyValueStoreSortedSetInterface needs to tell why and when it is useful, maybe with an example as well.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
14.73 KB
6.36 KB

Adding more docs / comments.

dixon_’s picture

Status: Needs review » Needs work

We will also need an update hook to install the new schema for existing sites.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
15.97 KB
1.43 KB

Adding upgrade path and test for it.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,57 @@
    + *
    + * An example would be an index of items, which have an order, where the last
    + * item or a range of items may be required.
    + */
    +interface KeyValueStoreSortedSetInterface {
    

    The documentation, at least for me, doesn't make it clear whether the values or the keys are sorted

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,57 @@
    +   * @param float $score
    +   *   The key for the item, for example microtime(), which can be used to
    +   *   generate a sequential value.
    

    It is weird that we just support floats for the key. Aren't also strings sortable?

  3. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,57 @@
    +   */
    +  public function add($score, $member);
    ...
    +   */
    +  public function addMultiple(array $pairs);
    

    Is there a reason why we don't have a chainable interface?

  4. +++ b/core/modules/system/system.install
    @@ -993,6 +993,37 @@ function system_schema() {
    +      // KEY is an SQL reserved word, so use 'name' as the key's field name.
    +      'name' => array(
    +        'description' => 'The index or score key for the value.',
    +        'type' => 'int',
    +        'not null' => TRUE,
    +        'default' => 0,
    +        'size' => 'big',
    +      ),
    

    I'm a bit confused because we document it as float but store an integer ...

  5. +++ b/core/modules/system/system.install
    @@ -993,6 +993,37 @@ function system_schema() {
    +    'indexes' => array(
    ...
    +      'name' => array('name'),
    +    ),
    

    Why would this index be needed? Is there any usecase without having a collection defined?

timmillwood’s picture

FileSize
16.02 KB
2.47 KB

#12.2 The key value sorted set is based on redis sorted set which uses a "score" as the key which is a float. We could be different and use strings, but I think Redis is a good model to follow. https://redis.io/topics/data-types-intro#redis-sorted-sets

All other items have been addressed.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -16,12 +16,16 @@
       /**
        * @param array $pairs
        *   An array of key/value pairs to add.
    +   *
    +   * @return $this
        */
       public function addMultiple(array $pairs);
    

    Many of those methods lack a one line description

  2. +++ b/core/modules/system/system.install
    @@ -1848,7 +1848,7 @@ function system_update_8300() {
           'name' => array(
             'description' => 'The index or score key for the value.',
    -        'type' => 'int',
    +        'type' => 'float',
             'not null' => TRUE,
             'default' => 0,
    

    Given this change we lack test coverage I believe

timmillwood’s picture

FileSize
17.16 KB
8.6 KB

Added a load of docblocks, tidy ups, and moved to integers everywhere, because floats suck!

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageSortedSet.php
    @@ -0,0 +1,142 @@
    + */
    +class DatabaseStorageSortedSet implements KeyValueStoreSortedSetInterface {
    +
    

    Most generic services, queue, cache etc. are lazy created now. Key_value is the only one which isn't, see #2664322: key_value table is only used by a core service but it depends on system install. I'm wondering though whether there is a reason that we cannot do it for this entirely new functionality.

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,73 @@
    + *
    + * An example would be an index of items, ordered by key, where the last
    + * item or a range of items may be required.
    + */
    +interface KeyValueStoreSortedSetInterface {
    

    This example still sounds really generic for me. Maybe a more real life example could help :) Also a pointer to redis could be really useful.

    I'm also wondering whether we should explain the concept of a set aka. each member can be in there just a single time, but the same score can be used multiple times.

  3. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,73 @@
    +   *   An array of key/value pairs to add.
    

    They are score/member pairs, right?

timmillwood’s picture

FileSize
17.95 KB
14.48 KB

Implementing changes suggested #16, mainly lazy creating the database table and changing all mentions of score/member to key/value.

timmillwood’s picture

An initial change record has been added. https://www.drupal.org/node/2841658

timmillwood’s picture

FileSize
17.95 KB
3.72 KB

Some tweaks and nit picks.

Status: Needs review » Needs work

The last submitted patch, 19: 2831428-19.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

Tests passing now so moving back to NCR.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageSortedSet.php
    @@ -0,0 +1,254 @@
    +   * @param string $table
    +   *   The name of the SQL table to use, defaults to key_value_sorted.
    ...
    +    $this->table = $table;
    

    Nitpick: Needs (optional)

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageSortedSet.php
    @@ -0,0 +1,254 @@
    +    foreach ($pairs as $pair) {
    +      foreach ($pair as $key => $value) {
    

    Note: You can get rid of one of the foreach by using a snippet like

    foreach ($a as list($b, $c)) {
    

    , see https://3v4l.org/qtb0j ... it is supported in PHP 5.5

  3. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageSortedSet.php
    @@ -0,0 +1,254 @@
    +            // If the exception happened for other reason than the missing bin
    ...
    +        // Now that the bin has been created, try again if necessary.
    

    'bin' is a term c&pasted from the cache implementation :)

  4. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageSortedSet.php
    @@ -0,0 +1,254 @@
    +      return $this->connection->select($this->table, 't')
    +        ->fields('t')
    +        ->condition('collection', $this->collection)
    +        ->countQuery()
    +        ->execute()
    +        ->fetchField();
    ...
    +  public function getMaxKey() {
    +    try {
    +      $query = $this->connection->select($this->table);
    +      $query->condition('collection', $this->collection, '=');
    +      $query->addExpression('MAX(name)');
    +      return $query->execute()->fetchField();
    +    }
    +    catch(\Exception $e) {
    +      $this->catchException($e);
    ...
    +  public function getMinKey() {
    +    try {
    +      $query = $this->connection->select($this->table);
    +      $query->condition('collection', $this->collection, '=');
    +      $query->addExpression('MIN(name)');
    +      return $query->execute()->fetchField();
    +    }
    +    catch(\Exception $e) {
    +      $this->catchException($e);
    

    Maybe a weird question, but why do we need DBTNG here? This query is fully static. Note: \Drupal\Core\KeyValueStore\DatabaseStorage also uses static methods.

  5. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
    @@ -0,0 +1,74 @@
    +  /**
    +   * Get the number of items in a collection.
    +   *
    +   * @return int
    +   *   The number of items in a collection.
    +   */
    +  public function getCount();
    
    +++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/DatabaseStorageSortedSetTest.php
    @@ -0,0 +1,149 @@
    +    $this->assertRecords(5, 'Correct number of record in the collection after value update.');
    

    Is there a reason we cannot use the getCount() method here? This would also increase the test coverage ...

  6. +++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/DatabaseStorageSortedSetTest.php
    @@ -0,0 +1,149 @@
    +    $this->assertSame(count($result), $expected_count, "Query affected $expected_count records.");
    

    Nitpick: There is $this->assertCount()

  7. +++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/DatabaseStorageSortedSetTest.php
    @@ -0,0 +1,149 @@
    +    $value = $this->store->getRange($key1, $key2);
    +    $this->assertSame($value, [$value1, $value2, $value3, $value4]);
    ...
    +    $value = $this->store->getRange($key1);
    +    $this->assertSame($value, [$value1, $value2, $value3, $value4]);
    ...
    +    $value = $this->store->getRange($new1, $new1);
    +    $this->assertSame($value, [$value1], 'Value was successfully updated.');
    ...
    +    $value = $this->store->getRange($key1, $key1);
    +    $this->assertSame($value, [], 'Non-existing range returned empty array.');
    ...
    +    $max_key = $this->store->getMaxKey();
    +    $this->assertEquals($max_key, $new1, 'The getMaxKey method returned correct key.');
    ...
    +    $min_key = $this->store->getMinKey();
    +    $this->assertEquals($min_key, $key0, 'The getMinKey method returned correct key.');
    

    Note: The expected value should be on the first, not the second parameter.

  8. +++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/DatabaseStorageSortedSetTest.php
    @@ -0,0 +1,149 @@
    +  }
    +}
    

    Nitpick: Needs a new line

timmillwood’s picture

FileSize
17.78 KB
6.55 KB

Addressed all the things in #22 apart from #22.2 because that doesn't work.

dawehner’s picture

@timmillwood Does it mean that we have multiple levels of pairs? Do you understand why my 3v4l snippet worked?

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
@@ -0,0 +1,74 @@
+  /**
+   * Add multiple items to a collection.
+   *
+   * @param array $pairs
+   *   An array of key/value pairs to add.
+   *
+   * @return $this
+   */
+  public function addMultiple(array $pairs);

I wonder whether we could add an example function call here, to show how to use it.

timmillwood’s picture

The 3v4l snippet didn't work because it was $a = [[1, 2], [2, 3]]; when it should've been $a = [[1 => 2], [2 => 3]];.

dawehner’s picture

The 3v4l snippet didn't work because it was $a = [[1, 2], [2, 3]]; when it should've been $a = [[1 => 2], [2 => 3]];.

Well in this case, the documentation or the implementation is bad. A pair is a pair of numbers (A, B) :) A pair, at least for me, is not a hashmap. Is there a requirement to keep the syntax like that? No matter what, we need at least an example in that piece of documentation :)

dawehner’s picture

Status: Needs review » Needs work

Needs work for fixing the documentation and or code :)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
17.83 KB
812 bytes

Changed the wording, variable and given an example.

dawehner’s picture

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreSortedSetInterface.php
@@ -27,12 +27,14 @@ public function add($key, $value);
+   * For example [[1 => 'a'], [2 => 'b']].

You can use @codeexample for that.

Honestly I'm wondering whether this is really the better API. [[1, 'a'], [2, 'b']] feels a bit better? Not sure at all to be honest.

timmillwood’s picture

FileSize
17.83 KB
708 bytes

Updated with @example.
I still like [$key => $value] for adding things to a key/value store.
Added array[] type hint as suggested on IRC.

jeqq’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

Is there a workflow issue open that needs this?

timmillwood’s picture

This is needed for workspaces (#2784921: Introduce the concept of workspaces) for the sequence index (#2826629: Create a sequence index).

catch’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

OK since that's going to be an experimental module, and it's still in progress, let's add the service + code to the experimental module and see how it goes there.

If it's needed and stable in its own right, we can move it to \Drupal\Core as part of the workflow module becoming stable. Marking this as duplicate of #2784921: Introduce the concept of workspaces.