[GitHub] [geode] sabbey37 commented on a change in pull request #6325: GEODE-9155: change frequency of passive expiration

[GitBox <gi...@apache.org>]

sabbey37 commented on a change in pull request #6325:
URL: https://github.com/apache/geode/pull/6325#discussion_r615105398



##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/resources/0001-configure-redis-tests.patch
##########
@@ -162,17 +162,33 @@ index de24eabed..aeeb1da7f 100644
  
      test {PERSIST returns 0 against non existing or non volatile keys} {
          r set x foo
-@@ -154,39 +154,39 @@ start_server {tags {"expire"}} {
-         set size1 [r dbsize]
-         # Redis expires random keys ten times every second so we are
-         # fairly sure that all the three keys should be evicted after
+@@ -149,44 +149,44 @@ start_server {tags {"expire"}} {
+-    test {Redis should actively expire keys incrementally} {
+-        r flushdb
+-        r psetex key1 500 a
+-        r psetex key2 500 a
+-        r psetex key3 500 a
+-        set size1 [r dbsize]
+-        # Redis expires random keys ten times every second so we are
+-        # fairly sure that all the three keys should be evicted after
 -        # one second.
 -        after 1000
-+        # two seconds.
-+        after 2000
-         set size2 [r dbsize]
-         list $size1 $size2
-     } {3 0}
+-        set size2 [r dbsize]
+-        list $size1 $size2
+-    } {3 0}
++#    test {Redis should actively expire keys incrementally} {
++#        r flushdb
++#        r psetex key1 500 a
++#        r psetex key2 500 a
++#        r psetex key3 500 a
++#        set size1 [r dbsize]
++#        # Redis expires random keys ten times every second so we are
++#        # fairly sure that all the three keys should be evicted after
++#        # one second.
++#        after 1000

Review comment:
       Is there any value in verifying the keys have expired after 3 minutes and 1 second (181,000 milliseconds) [adding a 1 second buffer in case passive expiration just ran]?  The tests only take about 9 minutes in CI right now, so adding an additional 3 minutes would not be a burden.  Maybe it's not necessary though since we have this test:
   ```
     @Test
     public void should_passivelyExpireKeys() {
       jedis.sadd("key", "value");
       jedis.pexpire("key", 100);
   
       GeodeAwaitility.await().until(() -> jedis.keys("key").isEmpty());
     }
     ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org