GEODE-9329: Implement radish leaderboard data structs #6548

ringles · 2021-06-02T14:37:25Z

Adds the OrderStatisticsTree/Set data type to support Redis-style Sorted Set data types and operations. To facilitate testing, this also implements the ZRANK command. (And thus, will remove the need for GEODE-9183.)

onichols-pivotal

the inclusion of OrderStatisticTree appears to comply with the ASF's TREATMENT OF THIRD-PARTY WORKS guidelines

upthewaterspout · 2021-06-02T17:50:16Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

 import org.apache.geode.redis.internal.netty.Coder;

 public class RedisSortedSet extends AbstractRedisData {
  private Object2ObjectOpenCustomHashMap<byte[], byte[]> members;


Should this be Object2DoubleOpenCustomHashMap?

This hashmap is used for quick lookup of scores associated with members. The score values are not used for sorting, they are just sent back to the client. So we're using Object2Object of byte[]'s to avoid the cost of converting to/from Doubles.

(We do convert to double for the OrderStatistics[Tree|Set], because we have to for sorting.)

I wonder if it might be better in the long run to have the members map be <byte[], OrderedSetEntry>. My reasoning is that with the current implementation, every time we do a zrank we create a new OrderedSetEntry just to get the index of an equal but already existing OrderedSetEntry in the scoreSet. If we added the OrderedSetEntry we create when we do a zadd to both data structures, then we could avoid an extra allocation for each zrank (and possibly other future commands) at the expense of a little extra memory footprint. I think for this to work properly though, we'd need both a Double score and byte[] scoreBytes field in OrderedSetEntry otherwise the whole point of the map would be kind of defeated.

Hmm. We could avoid storing the byteArray in the OrderedSetEntry, and pay the performance cost of converting from Double to byteArray for things like ZSCORE. But in this case it's probably better to spend a little memory overhead, in order to avoid every bit of performance drag we can.

upthewaterspout · 2021-06-02T17:51:31Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+
+  static class OrderedSetEntry implements Comparable<OrderedSetEntry> {
+    public byte[] member;
+    public Double score;


Should this be a double (lowercase, primitive)?

These members can be private and final I think.

Good call on the private and final. However, when sorting we use Double's compareTo(), which is not available on the double primitive.

upthewaterspout · 2021-06-02T17:58:21Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+      this.score = makeDoubleWhileHandlingInfinity(score);
+    }
+
+    private Double makeDoubleWhileHandlingInfinity(byte[] score) {


I wonder if these conversions belong in the Coder class? I could see having double Coder.bytesToDouble(byte[]) and byte[] Coder.doubleToBytes(double) methods. Lower case primitive doubles please!

Sure; Coder would need to handle +- infinity but that kind of implementation detail is best encapsulated in a class like that.

Coder already has bytesToDouble() and doubleToBytes() methods, so there's no need to reimplement that here. It's possible that the implementation in Coder needs a tweak, but I think Hale may be working on that as part of #6534

I am not making an changes to coder. I have a spike on these changes, but they depend on this PR being merged first. So I'll pick that up once this is in.

lgtm-com · 2021-06-02T18:28:19Z

This pull request introduces 1 alert when merging 5ff0749 into 44e5d4e - view on LGTM.com

new alerts:

1 for Inconsistent equals and hashCode

lgtm-com · 2021-06-02T19:25:29Z

This pull request introduces 1 alert when merging 19a9fa6 into 44e5d4e - view on LGTM.com

new alerts:

1 for Inconsistent equals and hashCode

upthewaterspout · 2021-06-02T20:14:16Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java

+  }
+
+  @Override
+  public boolean equals(Object o) {


I mentioned this earlier, but leaving this here as well. hashCode() and equals() for this class should obey the set contract. See the javadocs for Set.equals() and Set.hashCode()

They're tweaked, and pass all existing tests on my machine. We'll see what LGTM has to say.

lgtm-com · 2021-06-02T22:19:15Z

This pull request introduces 1 alert when merging fda8b61 into e39c4c5 - view on LGTM.com

new alerts:

1 for Inconsistent equals and hashCode

nonbinaryprogrammer

well done with all of this! as far as I can tell the tree handling stuff is solid, I've just got a couple of questions and suggestions

.../distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZRankDUnitTest.java

nonbinaryprogrammer · 2021-06-03T18:40:42Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+
+  @Test
+  public void shouldReturnNil_givenNonexistentMember() {
+    jedis.zadd("key", 1.0, "member");


it would be nice to pull "key" and "member" into constants since they're used all over

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

...ith-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java

...-redis/src/test/java/org/apache/geode/redis/internal/collections/OrderStatisticTreeTest.java

nonbinaryprogrammer · 2021-06-03T22:26:21Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+      this.score = makeDoubleWhileHandlingInfinity(score);
+    }
+
+    private Double makeDoubleWhileHandlingInfinity(byte[] score) {


I am not making an changes to coder. I have a spike on these changes, but they depend on this PR being merged first. So I'll pick that up once this is in.

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

nonbinaryprogrammer · 2021-06-03T22:30:26Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java

+ * @author Rodion "rodde" Efremov
+ * @version 1.6 (Feb 11, 2016)
+ */
+@SuppressWarnings("all")


Is there any way to be a little more targeted in suppressing warnings? this seems overly forgiving

Agreed. I would like to see this suppression removed and if there's no way to fix one of the warnings in this file, to suppress only that warning.

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

sabbey37

I won't be able to do a full review today, but I wanted to make some initial comments. Also, for the PR title/commit message, could we follow the convention described in our wiki? (so, GEODE-9329: Implement...)

sabbey37 · 2021-06-07T18:02:22Z

LICENSE

+  - OrderStatisticTree (https://github.com/coderodde/OrderStatisticTree)
+    Copyright (c) 2021 Rodion Efremov


Small thing, but I was wondering if there should be a comma before Copyright to be consistent with the lines above it.

Good catch!

sabbey37 · 2021-06-07T18:03:55Z

.../distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZRankDUnitTest.java

+  private static final String LOCAL_HOST = "127.0.0.1";
+  private static final String KEY = "key";
+  private static final int SET_SIZE = 1000;
+  private static final int JEDIS_TIMEOUT =


There are constants in RedisClusterStartupRule that could be used here in lieu of LOCAL_HOST and JEDIS_TIMEOUT.

Jens and I reworked the DUnit tests, and got rid of all but ZAddDUnitTest and ZRemDUnitTest. But I'm using those constants in those tests now.

sabbey37 · 2021-06-07T18:15:35Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+  private static final int REDIS_CLIENT_TIMEOUT =
+      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());


There are constants in RedisClusterStartupRule that could be used here in lieu of REDIS_CLIENT_TIMEOUT.

sabbey37 · 2021-06-07T18:17:03Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+
+  @Test
+  public void shouldError_givenWrongNumberOfArguments() {
+    assertExactNumberOfArgs(jedis, Protocol.Command.ZSCORE, 2);


Should this be ZRANK instead of ZSCORE?

Dang you're good!

sabbey37 · 2021-06-07T18:18:50Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+
+    // get the ranks of the members
+    Iterator<String> membersIterator = map.keySet().iterator();
+    String[] members = new String[10];


Could we reuse the ENTRY_COUNT variable here?

Yeah, in a couple places.

sabbey37 · 2021-06-07T18:24:58Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+      rankMap.put(rank, memberName.getBytes(StandardCharsets.UTF_8));
+      memberList.add(memberName.getBytes(StandardCharsets.UTF_8));


Could we use the Coder.stringToBytes() method for these instead of .getBytes? I think we're trying to make that consistent throughout the Radish module.

Yeah, updated. Thanks!

sabbey37 · 2021-06-07T18:58:21Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java

-            Double.valueOf(subCommandString);
-          } catch (NumberFormatException nfe) {
+          // Use regex to validate score is a valid floating point number
+          if (!subCommandString.matches("^[-+]?[0-9]*\\.?[0-9]+([eE][-+]?[0-9]+)?$")) {


Is there an advantage to using the regex over Double.parseDouble? If not, it seems clearer and less error-prone to just try parsing the double. I realize we ignore the output of that though.

Well, the intent is to avoid the whole conversion, and just do the regex. Looking at, e.g. OpenJDK's implementation of parseDouble, though, they pretty much do a regex anyway. There might possibly be one extra stack frame, but it shouldn't matter particularly. So I swapped it for just Double.parseDouble().

sabbey37 · 2021-06-07T19:02:07Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java

+        case "-inf":
+        case "inf":
+        case "+inf":
+          scoreFound = true;
+          break;


It would be good to rebase now that @nonbinaryprogrammer's PR for infinity handling in ZADD is merged.

Did that this morning. Hopefully things will be cleaner.

DonalEvans

A lot of comments inline, sorry if it's overwhelming. Would it also be possible to rearrange the contents of OrderStatisticsTree a little so that fields are at the top rather than a quarter of the way down the class, and so that inner classes appear at the bottom? it would improve the readability a lot, I think.

.../distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZRankDUnitTest.java

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

DonalEvans · 2021-06-07T22:03:17Z

...-redis/src/test/java/org/apache/geode/redis/internal/collections/OrderStatisticTreeTest.java

+  }
+
+  @Test
+  public void testConcurrentOrIllegalStateOnRemove() {


This test name seems to imply the test is doing something different from what it's actually doing. The test throws an IllegalStateException because remove() is called before next() has been called, not because of the call to add(). If next() is called on each iterator before the add() call, then the calls to remove() throw ConcurrentModificationException, which is what I think the test was originally intended to show.

Yup, but the subsequent test does the ConcurrentModificationException test.

...-redis/src/test/java/org/apache/geode/redis/internal/collections/OrderStatisticTreeTest.java

DonalEvans

Just a few more things to clean up.

DonalEvans · 2021-06-10T23:16:36Z

...c/distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZAddDUnitTest.java

-    new ConcurrentLoopingThreads(SET_SIZE,
-        (i) -> jedis.zadd(key, memberScoreMap1),
-        (i) -> jedis.zadd(key, memberScoreMap2)).runInLockstep();
+    clusterStartUp.crashVM(3);


Could this 3 be tied directly to the 4 used to initialize the RedisClusterStartupRule?

Now relative to a constant.

DonalEvans · 2021-06-10T23:19:12Z

...c/distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZAddDUnitTest.java


    server1.stop();
    server2.stop();
    server3.stop();


It wasn't added by this PR, but since you're changing this file, it's not necessary to call stop() on the servers, as this is handled automatically by RedisClusterStartupRule. With these calls removed, the server1, server2 and server3 fields are no longer needed and can be removed entirely.

DonalEvans · 2021-06-10T23:23:08Z

...c/distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZAddDUnitTest.java

-      assertThat(score).isEqualTo(memberScoreMap2.get(member2));
-    }
-
+    throw lastException;


This could hit an NPE if maxRetries is less than 1. A check before the for loop can fix this:

if (maxRetries < 1) { maxRetries = 1; }

Handled with an assert. If someone ever fed it anything less than 1, it should be a failure condition.

DonalEvans · 2021-06-10T23:27:59Z

...st/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRankIntegrationTest.java

+    jedis.zadd(KEY, map);
+
+    // get the ranks of the members
+    Iterator<String> membersIterator = map.keySet().iterator();


This variable is no longer used and can be removed.

DonalEvans · 2021-06-10T23:39:42Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java

+    if (!c.getClass().equals(HashSet.class)) {
+      c = new HashSet<>(c);
+    }


Is there a reason that we're copying into a HashSet here? It should be fine to call contains() on any Collection.

No, it doesn't seem to be needed. We've already made enough changes from the upstream implementation, once this PR is accepted I'm hoping to do a PR back to the original dev.

DonalEvans · 2021-06-10T23:51:52Z

...ith-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java

+    if (insertionMode) {
+      // Whenever fixing after insertion, at most one rotation is
+      // required in order to maintain the balance.
+      return true;
+    }
+    return false;


This can be simplified to just return insertionMode;

Fair enough, but I kept the comment. :)

DonalEvans · 2021-06-10T23:54:27Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

    return getSortedSetSize() - initialSize + changesCount;
  }

+  private void validateScoreIsDouble(byte[] score) {


This method is no longer used and can be removed.

Yeah, that's a leftover from the multiple rebases, thanks for catching it!

DonalEvans

Thanks for addressing all the feedback!

Adding the OrderedStatisticsTree from https://github.com/coderodde/OrderStatisticTree to the geode codebase. Adding the appropriate information to the LICENSE file. Adding a new quick check test and benchmark for this class

…ations/conversions

…her cleanup

…icTreeTest

DonalEvans

Just a couple of small comments after the rebase.

DonalEvans · 2021-06-21T22:14:18Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+    if (isPositiveInfinity(value)) {
+      return POSITIVE_INFINITY;
+    }
+    if (isNegativeInfinity(value)) {
+      return NEGATIVE_INFINITY;
+    }


Would it make sense to move this implementation into Coder.bytesToDouble() so that it's available everywhere?

DonalEvans · 2021-06-21T22:16:26Z

geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java

+    if (bytes == null) {
+      return false;
+    }


This null check is not necessary, as equalsIgnoreCaseBytes() handles nulls.

Would it therefore also not be necessary in isPositiveInfinity() and isNegativeInfinity() below it?

Yeah, good catch, thanks for cleaning that up.

…update CoderTest for NaN handling

DonalEvans · 2021-06-22T16:43:04Z

geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java

+      return NEGATIVE_INFINITY;
+    }
+    if (isNaN(bytes)) {
+      throw new NumberFormatException(ERROR_NOT_A_VALID_FLOAT);


Strictly speaking, I think this should return Double.NaN rather than throwing, as that is a valid value that a Double can take. It'll be the responsibility of the caller to determine what to do with the NaN value in that case.

Approving reviews available

ringles requested review from DonalEvans, dickcav, jdeppe-pivotal, nonbinaryprogrammer, onichols-pivotal, sabbey37 and upthewaterspout as code owners June 2, 2021 14:37

onichols-pivotal approved these changes Jun 2, 2021

View reviewed changes

dickcav approved these changes Jun 2, 2021

View reviewed changes

upthewaterspout reviewed Jun 2, 2021

View reviewed changes

jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Jun 3, 2021

nonbinaryprogrammer previously requested changes Jun 3, 2021

View reviewed changes

sabbey37 reviewed Jun 7, 2021

View reviewed changes

DonalEvans requested changes Jun 7, 2021

View reviewed changes

nonbinaryprogrammer changed the title ~~Geode 9329 implement radish leaderboard data structs~~ GEODE-9329: implement radish leaderboard data structs Jun 8, 2021

ringles changed the title ~~GEODE-9329: implement radish leaderboard data structs~~ GEODE-9329: Implement radish leaderboard data structs Jun 9, 2021

DonalEvans mentioned this pull request Jun 10, 2021

GEODE-9301: Implement accurate sizeable for RedisHash #6590

Merged

5 tasks

DonalEvans requested changes Jun 11, 2021

View reviewed changes

DonalEvans approved these changes Jun 11, 2021

View reviewed changes

upthewaterspout and others added 7 commits June 21, 2021 09:48

Adding the OrderedStatisticsTree to the geode codebase

3fb1d6e

Adding the OrderedStatisticsTree from https://github.com/coderodde/OrderStatisticTree to the geode codebase. Adding the appropriate information to the LICENSE file. Adding a new quick check test and benchmark for this class

Implemnent equals() on OrderedSetTree, add to RedisSortedSet

13a33d2

Initial support for ZRANK

7281ad0

WIP Basic ranking works with unique members and scores

bc9ac4d

WIP integration tests passing lexical and numeric sort

0af960c

WIP spA and fix test support function

325f2ad

Add DUnit tests, ignore sizable tests for now

7bf78bd

ringles added 19 commits June 21, 2021 09:52

Ill-advised change after CH rebase

2ab80e3

Rework DUnit test to confirm data distribution across members

6cde28c

Merge cleanup, DUnit test tweaks

7c80d1b

Update 'members' map to store OrderedSetEntry to avoid unneeded alloc…

85ad9bd

…ations/conversions

Fix warnings, remove code duplication, fix tests and add new test, ot…

694f197

…her cleanup

Update OrderStatisticsTree, fix warnings, refactor duplicate code

3e498ac

More OrderStatisticsTree cleanup, use assertJ throughout OrderStatist…

cf5e0c7

…icTreeTest

Refactor test for clarity, use assertThatThrownBy() throughout

d83396a

Update to handle function invocation rework

e0db6e1

Slight change in how BIND_ADDRESS is imported

c6a1bfe

Much test polish

cdf26ff

More test polish

a89ef5e

Remove one last unused import

8421263

Remove dead code from refactor

e34c7c1

Clean up some unused code, some polish

c25f531

Ensure double conversion routine is static

1413143

Refine double parsing using new byte array Coder methods

bbcd28e

CMD-shift-o is not a substitute for spA

8564e58

Add NaN handling to fit new double validation scheme

7c2fea4

DonalEvans reviewed Jun 21, 2021

View reviewed changes

ringles added 2 commits June 22, 2021 10:56

Update bytesToDouble to handle INF/NaN, remove unneeded null checks, …

51666da

…update CoderTest for NaN handling

A day at the spA

e42b69c

DonalEvans reviewed Jun 22, 2021

View reviewed changes

ringles requested review from nonbinaryprogrammer and removed request for nonbinaryprogrammer June 22, 2021 18:10

Return NaN from Coder.bytesToDouble and let caller handle

5324bda

DonalEvans approved these changes Jun 22, 2021

View reviewed changes

ringles merged commit 4f106c6 into apache:develop Jun 22, 2021

onichols-pivotal requested a review from metatype June 26, 2021 00:33

		- OrderStatisticTree (https://github.com/coderodde/OrderStatisticTree)
		Copyright (c) 2021 Rodion Efremov

		private static final int REDIS_CLIENT_TIMEOUT =
		Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());

		rankMap.put(rank, memberName.getBytes(StandardCharsets.UTF_8));
		memberList.add(memberName.getBytes(StandardCharsets.UTF_8));

GEODE-9329: Implement radish leaderboard data structs #6548

GEODE-9329: Implement radish leaderboard data structs #6548

Uh oh!

Conversation

ringles commented Jun 2, 2021

Uh oh!

onichols-pivotal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 2, 2021

Uh oh!

lgtm-com bot commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 2, 2021

Uh oh!

nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sabbey37 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sabbey37 left a comment •

edited

Loading