GEODE-9953: Implement LTRIM Command #7403

ringles · 2022-02-28T20:36:13Z

GEODE-9953: Implement LTRIM Command

Implements the Redis LTRIM command using the new versioning scheme.

jdeppe-pivotal · 2022-03-04T13:27:27Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+    synchronized (this) {
+      if (version != this.version) {
+        elementsRemove(indexes);
+      }
+      this.version = version;


There shouldn't be any need to synchronize here since this is a delta application and there will be locking at the Geode level to protect the entry. ~~I think this should just be:~~

< Deleted 'cos I'm an idiot>

~~You want to update the version to match the change being applied. Similarly in applyAddByteArrayVersionedDelta.~~

jdeppe-pivotal · 2022-03-15T21:10:37Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+        assertThat(jedis.llen(key)).isEqualTo(elementCount);
+        elementCount--;
+        i++;
+      } catch (Exception ex) {


If you want a failing assertion to stop this loop (and thus the test) you'll have to catch a Throwable here. Prefer rather to add an .as clause to the assertion with the details you're adding to the RuntimeException and just rethrow the caught exception.

jdeppe-pivotal · 2022-03-15T22:38:52Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPushDUnitTest.java

+      assertThat(length).isCloseTo(ITERATION_COUNT * 2 * PUSH_LIST_SIZE, within(6L));
+      assertThat(length % 3).isEqualTo(0);


I think this assertion can be exact and shouldn't need the offset.

Agreed, this assertion should not be using isCloseTo() since we should not be seeing duplicated commands here.

I have seen failures, but that was an earlier version. Ran a bunch of these without failures, so I'm making it exact.

...edis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LTrimExecutor.java

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

...edis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LTrimExecutor.java

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java

DonalEvans

This PR should also add LTRIM to the list of supported commands in geode_for_redis.html.md.erb

DonalEvans · 2022-03-22T17:05:53Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPushDUnitTest.java

+      assertThat(length).isCloseTo(ITERATION_COUNT * 2 * PUSH_LIST_SIZE, within(6L));
+      assertThat(length % 3).isEqualTo(0);


Agreed, this assertion should not be using isCloseTo() since we should not be seeing duplicated commands here.

DonalEvans · 2022-03-22T17:11:41Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPushDUnitTest.java

      assertThat(length % 3).isEqualTo(0);
      validateListContents(key, length, keyToElementListMap);
    }
  }

  private void lpushPerformAndVerify(String key, List<String> elementList,
      AtomicLong runningCount) {
-    for (int i = 0; i < MINIMUM_ITERATIONS; i++) {
+    for (int i = 0; i < ITERATION_COUNT; i++) {


Rather than having this test always do a fixed number of LPUSH operations and moving buckets while they're ongoing, it would be better to have a fixed number of bucket moves performed (25 seems like a reasonable number) and have LPUSH happening continuously until those bucket moves are finished, since what we really want to test here is how LPUSH behaves when we move buckets, so we should make sure that we always do the same number of bucket moves.

This can be achieved by replacing the for loop in this method with a while loop, having an AtomicBoolean rather than an AtomicInteger as the condition (named something like continueRunning maybe), and setting that AtomicBoolean to false after the for loop that moves buckets completes. A similar approach is used in LInsertDUnitTest.

DonalEvans · 2022-03-22T17:14:51Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPushDUnitTest.java

+    length = jedis.llen(key);
+    while (length > 0) {
      List<String> elementList = keyToElementListMap.get(key);
      assertThat(jedis.lpop(key)).isEqualTo(elementList.get(2));
      assertThat(jedis.lpop(key)).isEqualTo(elementList.get(1));
      assertThat(jedis.lpop(key)).isEqualTo(elementList.get(0));
+      length = jedis.llen(key);


This change seems unnecessary. It would probably be better to just remove length from the method signature and keep the while loop as it was.

DonalEvans · 2022-03-22T17:20:37Z

...edis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LTrimExecutor.java

+    Region<RedisKey, RedisData> region = context.getRegion();
+    RedisKey key = command.getKey();


Minor performance improvement, but these lines should be moved to below the try/catch as it's possible we return from this method before needing them.

Minorly updated.

DonalEvans · 2022-03-22T17:25:49Z

...edis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LTrimExecutor.java

+    long start;
+    long end;
+
+    try {
+      start = Coder.bytesToLong(commandElems.get(startIndex));
+      end = Coder.bytesToLong(commandElems.get(stopIndex));


Having these values be long here but then casting them to int inside the ltrim() method could potentially lead to overflows, so it might be safer to wrap these calls to bytesToLong() in narrowLongToInt() to safely convert from long to int and have the values be int throughout.

DonalEvans · 2022-03-22T18:50:09Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

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


I'm not super clear on what this test is showing, since we already have a test that shows that the key is removed if all elements are trimmed, and a test that shows the behaviour when LTRIM is called on a non-existent key. I think this can probably just be removed.

Yeah, really only there during development for TDD.

DonalEvans · 2022-03-22T18:54:25Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+    final AtomicReference<String> lpopReference = new AtomicReference<>();
+    new ConcurrentLoopingThreads(1000,
+        i -> jedis.lpush(KEY, valuesToAdd),
+        i -> jedis.ltrim(KEY, 0, 2),
+        i -> lpopReference.set(jedis.lpop(KEY)))
+            .runWithAction(() -> {
+              assertThat(lpopReference).satisfiesAnyOf(
+                  lpopResult -> assertThat(lpopReference.get()).isEqualTo("orange"),
+                  lpopResult -> assertThat(lpopReference.get()).isEqualTo("troix"));
+              jedis.del(KEY);
+              jedis.lpush(KEY, valuesInitial);
+            });


The call to LPOP here should be removed, since we're not trying to test the concurrent behaviour of LPUSH, LTRIM and LPOP. With the test as it is, we could do the push, then do the pop, then do the trim, which is not what we're trying to test.

Instead, we should be calling LRANGE in the runWithAction lambda and asserting that the contents of the list is either {"un", "deux", "trois"} if the LPUSH happened first or {"plum", "peach", "orange"} if the LTRIM happened first (but with the elements in reverse order, because LPUSH likes to complicate things).

When the test was written, we didn't have LRANGE. Reworked.

DonalEvans · 2022-03-22T18:57:40Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+    // Remove all but last element
+    jedis.ltrim(key, INITIAL_LIST_SIZE - 1, INITIAL_LIST_SIZE);


To make this test a bit more robust, it would be good to select start and end indexes such that we remove some elements from the start and some elements form the end of the list. That way we're checking that we're using both start and end indexes correctly in the Delta that we send.

Significantly reworked.

DonalEvans · 2022-03-22T19:02:35Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+
+  @Test
+  public void givenBucketsMoveDuringLtrim_thenOperationsAreNotLost() throws Exception {
+    AtomicLong runningCount = new AtomicLong(3);


This test would be better if instead of running until we've done a certain number of LTRIM calls, we instead do constant LTRIM calls until we've done a certain number of bucket moves. The LRemDUnitTest that does bucket moves that's being added as part of the LREM PR is a great example of how we want to be approaching these sort of tests, I think, so it could be helpful to use it as a guide.

The basic idea is that we do our operation constantly until we're finished moving buckets, but since LTRIM removes elements, we need to reset the list to its initial state once it's been emptied, then continue calling LTRIM. That way we can be sure that we do enough bucket moves to make the test consistent and useful, without having to worry that we run out of elements to trim.

DonalEvans · 2022-03-22T19:06:06Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+        jedis.ltrim(key, 0, lastElementIndex - 1);
+        assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(lastElementIndex);
+        lastElementIndex--;


Could we choose indexes for start and end here so that we're keeping some middle part of the list instead of just trimming the last element? That way we make sure that we're exercising the whole of the Delta that we send, not just the end index. Maybe just trim the first and last elements?

Also, it would be good to assert on the entire contents of the list (using LRANGE) rather than just the size, as we want to make sure that we're not removing the wrong elements.

Got a test that removes the first and last each step, and verifies the values along the way. (Didn't have LRANGE when the test was first written.)

nonbinaryprogrammer

I did not re-review all the changes, but the concerns/suggestions that I raised have been addressed. I am on vacation next week so I'm going to go ahead and approve so that my review doesn't hold up merging.

jdeppe-pivotal

Looks good. One minor optional suggestion...

jdeppe-pivotal · 2022-03-28T21:00:41Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+        try {
+          assertThat(jedis.lindex(key, 0)).isEqualTo(makeElementString(key, INITIAL_LIST_SIZE - i));
+          assertThat(jedis.lindex(key, lastIndex)).isEqualTo(makeElementString(key, i));
+          jedis.ltrim(key, 1, lastIndex);
+          assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(lastIndex);
+        } catch (Exception ex) {
+          isRunning.set(false); // test is over
+          throw new RuntimeException("Exception performing LTRIM for list '"
+              + key + "' at step " + i + ": " + ex.getMessage());
+        }
+      }


Exceptions thrown by the asserts won't be caught by the catch block and so won't stop the test. Not critical, but you could change this to:

try { assertThat(jedis.lindex(key, 0)) .as("lpush head verification failed at iteration " + i) .isEqualTo(makeElementString(key, INITIAL_LIST_SIZE - i)); assertThat(jedis.lindex(key, lastIndex)) .as("lpush tail verification failed at iteration " + i) .isEqualTo(makeElementString(key, i)); jedis.ltrim(key, 1, lastIndex); assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(lastIndex); } catch (Throwable ex) { isRunning.set(false); // test is over throw ex; }

Now I get what you mean by the throwable comment earlier. Done.

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

lgtm-com · 2022-03-28T22:38:23Z

This pull request fixes 1 alert when merging ac9b48b into dfedcbd - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

lgtm-com · 2022-03-29T14:36:28Z

This pull request fixes 1 alert when merging 18c118a into 41844a5 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

DonalEvans

This PR also needs to update the docs in geode_for_redis.html.md.erb. The README.md file is intended for developers, not users (and I'm pretty sure we have a story somewhere to just delete it) so it doesn't really need to be updated with every supported command, since a developer could just look in RedisCommandType.

DonalEvans · 2022-03-29T18:18:30Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+    int length = elementList.size();
+    int boundedStart = getBoundedStartIndex(start, length);
+    int boundedEnd = getBoundedEndIndex(end, length);
+
+    if (boundedStart > boundedEnd || boundedStart == length) {
+      // Remove everything
+      region.remove(key);
+      return null;


It might be worth adding an implementation of ltrim() to NullRedisList that just returns immediately, since then we can avoid doing these steps. I'm not sure how expensive a call to region.remove() for a non-existent key is, but it's probably worth avoiding if we can.

DonalEvans · 2022-03-29T18:34:44Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+   * @return the element actually popped
+   */
+  public byte[] ltrim(long start, long end, Region<RedisKey, RedisData> region,


I think the method return type being Void here would still be a nice improvement, since it's misleading to imply that a byte[] is returned when this method always returns null. Also, start and end are now passed in as int, so the method signature (and the signatures of getBoundedStartIndex() and getBoundedEndIndex()) should be changed to reflect that.

DonalEvans · 2022-03-29T19:37:43Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+  private int getBoundedEndIndex(long index, int size) {
+    if (index >= 0L) {
+      return (int) Math.min(index, size);
+    } else {
+      return (int) Math.max(index + size, -1);
+    }


This method is still inconsistent, since for a list of size n, passing in n as the index returns a different value than passing in -1, when they should be equivalent. This can lead to us failing to hit the early return in ltrim() when the operation is a no-op and sending a delta that we don't need to.

This method should return the exclusive index to be used in clearSublist() so that we can be consistent with how Java deals with indexes and not have to add 1 to the value before passing it into other methods, i.e. it should return size -1 if the index passed to it is greater than or equal to size, or if the index is -1.

DonalEvans · 2022-03-29T19:40:23Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+      if (boundedEnd < length) {
+        // trim stuff at end of list
+        elementList.clearSublist(boundedEnd + 1, length);
+      }
+      if (boundedStart > 0) {
+        // trim stuff at start of list
+        elementList.clearSublist(0, boundedStart);
+      }


To prevent code duplication, this should probably be replaced with elementsRetainByIndexRange(boundedStart, boundedEnd);

DonalEvans · 2022-03-29T19:44:25Z

...is/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java

@@ -83,6 +83,39 @@
    return indexesRemoved;
  }

+  public void clearSublist(int fromIndex, int toIndex) {
+    if (fromIndex < size() / 2) {


This attempt at optimization is not working the way you intend, I think. For a list of 100 elements with fromIndex = 49, toIndex = 100, you end up starting at the beginning of the list and iterating all 100 elements, rather than starting at the end and only having to iterate 51. The correct optimization would be to check which of fromIndex and toIndex is closer to its respective end of the list, and then clear from that side.

DonalEvans · 2022-03-29T20:24:45Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+  public void removesKey_whenAllElementsTrimmed() {
+    initializeTestList();
+
+    jedis.ltrim(KEY, 0, -5);


This test could be expanded to "removesKey_whenRangeIsEmpty()" and parameterized to use various ranges that do not contain any elements e.g. both start and end are past the end of the list, both start and end are before the start of the list etc.

Parameterized.

DonalEvans · 2022-03-29T20:27:29Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest {


To help catch potential edge cases, could we have a parameterized test for the behaviour when the list contains only one element? I added some trace logging and played around a bit and noticed that there was some inconsistent/unexpected behaviour when the list contains only one element that it would be good to explicitly test.

Can you think of more cases to add to the new trimsToSpecifiedRange_givenListWithOneElement() test?

What you have looks comprehensive to me

DonalEvans · 2022-03-29T20:31:49Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+    if (boundedStart == 0 && boundedEnd == length) {
+      // No-op, return without modifying the list
+      return null;
+    }


It might be nice to add a test to RedisListTest.java to confirm that we hit this early return under various conditions, as I noticed that for lists with just one element, we were still sending a delta in some situations even though the list wasn't modified at all.

Checking this in tests now.

DonalEvans · 2022-03-29T20:58:46Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+      for (int i = 1; i <= INITIAL_LIST_SIZE / 2 && isRunning.get(); i++) {
+        long lastIndex = jedis.llen(key) - 2;
+        try {
+          assertThat(jedis.lindex(key, 0))
+              .as("lpush head verification failed at iteration " + i)
+              .isEqualTo(makeElementString(key, INITIAL_LIST_SIZE - i));
+          assertThat(jedis.lindex(key, lastIndex))
+              .as("lpush tail verification failed at iteration " + i)
+              .isEqualTo(makeElementString(key, i));
+          jedis.ltrim(key, 1, lastIndex);
+          assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(lastIndex);
+        } catch (Throwable ex) {
+          isRunning.set(false); // test is over
+          throw ex;
+        }
+      }
+      if (isRunning.get()) {
+        assertThat(jedis.exists(key)).isFalse();
+      }


This can be simplified somewhat by using negative indexes and doing the assertions after performing LTRIM (since we already assert on the contents of the list as part of lpushPerformAndVerify() so we should never see the list be wrong before we've even done an LTRIM):

for (int i = 1; i < INITIAL_LIST_SIZE / 2 && isRunning.get(); i++) { try { jedis.ltrim(key, 1, -2); assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(INITIAL_LIST_SIZE - (i * 2L)); assertThat(jedis.lindex(key, 0)) .as("LTRIM head verification failed at iteration " + i) .isEqualTo(makeElementString(key, INITIAL_LIST_SIZE - 1 - i)); assertThat(jedis.lindex(key, -1)) .as("LTRIM tail verification failed at iteration " + i) .isEqualTo(makeElementString(key, i)); } catch (Throwable ex) { isRunning.set(false); // test is over throw ex; } } if (isRunning.get()) { jedis.ltrim(key, 1, -2); assertThat(jedis.exists(key)).isFalse(); }

DonalEvans · 2022-03-29T20:59:45Z

...tributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java

+    Future<Void> future2 = executor.runAsync(task2);
+    Future<Void> future3 = executor.runAsync(task3);
+
+    for (int i = 0; i < 100; i++) {


in order to have the test stop early if ltrimPerformAndVerify() encounters an exception, you need to also include a check for isRunning.get() in the for loop condition.

Conditionalized.

DonalEvans

The docs (not the README) still need to be updated to include LTRIM in this PR.

DonalEvans · 2022-03-30T22:03:56Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

+  private int getBoundedStartIndex(long index, int size) {
+    if (index >= 0L) {
+      return (int) Math.min(index, size);
+    } else {
+      return (int) Math.max(index + size, 0);
+    }
+  }
+
+  private int getBoundedEndIndex(long index, int size) {


index in these two methods is always an int so the method signatures should be updated to reflect this and then the casts to int in the method can be removed.

lgtm-com · 2022-03-30T22:37:32Z

This pull request fixes 1 alert when merging 69ef004 into f3b9636 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

lgtm-com · 2022-03-31T17:52:41Z

This pull request fixes 1 alert when merging 9202fef into 6806179 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

DonalEvans · 2022-03-31T18:18:37Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+  @SuppressWarnings("unused")
+  private Object[] getRangesForOneElementList() {
+    // Values are start, end, expected
+    // For initial list of {e4, e3, e2, e1}


This comment is incorrect

Yanked, since it's pretty simple for a one-element list.

DonalEvans · 2022-03-31T18:19:30Z

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java

      return (int) Math.min(index, size);
    } else {
      return (int) Math.max(index + size, 0);


These casts are now redundant and can be removed, as are the ones in getBoundedEndIndex().

Swept away.

DonalEvans · 2022-03-31T18:21:08Z

geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/RedisListTest.java

+    RedisList list = createRedisListWithDuplicateElements();
+
+    byte originalVersion = list.getVersion();
+    list.ltrim(0, 5, region, null);


To make it more obvious what this is doing, could the end index be -1? That way, regardless of the size of the list, we're guaranteed to always do a no-op.

DonalEvans · 2022-03-31T18:22:48Z

geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/RedisListTest.java

+
+  private RedisList createRedisListWithOneElement(int e1) {
+    RedisList newList = new RedisList();
+    newList.elementPushHead(new byte[] {(byte) e1});


For simplicity, could this method just take a byte[] as the argument rather than an int that we then have to fiddle with to put into the list?

Since we don't care about the contents of the element, now there's no arg at all.

DonalEvans · 2022-03-31T18:23:46Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest {


What you have looks comprehensive to me

lgtm-com · 2022-04-01T14:27:31Z

This pull request fixes 1 alert when merging fc2fd52 into 5dce03c - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

lgtm-com · 2022-04-01T19:09:30Z

This pull request fixes 1 alert when merging f64db77 into 4a73cb7 - view on LGTM.com

fixed alerts:

1 for Spurious Javadoc @param tags

DonalEvans · 2022-04-04T18:05:27Z

...ava/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java

+    jedis.lpush(KEY, valuesInitial);
+    List<String> valuesInitialReversed = new ArrayList<>(Arrays.asList("trois", "deux", "un"));
+    List<String> valuesToAddReversed = new ArrayList<>(Arrays.asList("orange", "peach", "plum"));
+    final AtomicReference<String> lpopReference = new AtomicReference<>();


This variable is unused and should be removed.

jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Mar 1, 2022

jdeppe-pivotal reviewed Mar 4, 2022

View reviewed changes

ringles force-pushed the GEODE-9953-Implement-LTRIM-command branch 3 times, most recently from ce67f72 to 266c68e Compare March 10, 2022 23:14

ringles marked this pull request as ready for review March 11, 2022 13:03

ringles requested review from nonbinaryprogrammer, upthewaterspout, DonalEvans and dschneider-pivotal as code owners March 11, 2022 13:03

onichols-pivotal requested a review from davebarnes97 March 11, 2022 20:39

jdeppe-pivotal requested changes Mar 15, 2022

View reviewed changes

dschneider-pivotal requested changes Mar 15, 2022

View reviewed changes

nonbinaryprogrammer requested changes Mar 15, 2022

View reviewed changes

ringles force-pushed the GEODE-9953-Implement-LTRIM-command branch from 5be5ea3 to 737659d Compare March 22, 2022 14:39

DonalEvans requested changes Mar 22, 2022

View reviewed changes

ringles force-pushed the GEODE-9953-Implement-LTRIM-command branch from c6e8e66 to f5779ea Compare March 23, 2022 21:33

nonbinaryprogrammer approved these changes Mar 24, 2022

View reviewed changes

ringles force-pushed the GEODE-9953-Implement-LTRIM-command branch from 4266dda to 7bf0499 Compare March 28, 2022 14:18

jdeppe-pivotal approved these changes Mar 28, 2022

View reviewed changes

dschneider-pivotal requested changes Mar 28, 2022

View reviewed changes

geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java Outdated Show resolved Hide resolved

DonalEvans requested changes Mar 29, 2022

View reviewed changes

DonalEvans reviewed Mar 30, 2022

View reviewed changes

DonalEvans reviewed Mar 31, 2022

View reviewed changes

dschneider-pivotal approved these changes Apr 1, 2022

View reviewed changes

ringles added 2 commits April 1, 2022 14:24

GEODE-9953: Implement LTRIM command

678ef04

Docs updated

f64db77

ringles force-pushed the GEODE-9953-Implement-LTRIM-command branch from 2495f07 to f64db77 Compare April 1, 2022 18:28

DonalEvans approved these changes Apr 4, 2022

View reviewed changes

ringles merged commit 3cc3bdb into apache:develop Apr 4, 2022

ringles deleted the GEODE-9953-Implement-LTRIM-command branch April 4, 2022 19:35

		assertThat(length).isCloseTo(ITERATION_COUNT * 2 * PUSH_LIST_SIZE, within(6L));
		assertThat(length % 3).isEqualTo(0);

		Region<RedisKey, RedisData> region = context.getRegion();
		RedisKey key = command.getKey();

		// Remove all but last element
		jedis.ltrim(key, INITIAL_LIST_SIZE - 1, INITIAL_LIST_SIZE);

GEODE-9953: Implement LTRIM Command #7403

GEODE-9953: Implement LTRIM Command #7403

Conversation

ringles commented Feb 28, 2022 • edited

jdeppe-pivotal Mar 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

jdeppe-pivotal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 28, 2022

lgtm-com bot commented Mar 29, 2022

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 30, 2022

lgtm-com bot commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 1, 2022

lgtm-com bot commented Apr 1, 2022

Choose a reason for hiding this comment

ringles commented Feb 28, 2022 •

edited

jdeppe-pivotal Mar 4, 2022 •

edited