[GitHub] [geode] kirklund commented on a change in pull request #6926: GEODE-9607 take2

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

kirklund commented on a change in pull request #6926:
URL: https://github.com/apache/geode/pull/6926#discussion_r725207167



##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/OutOfMemoryDUnitTest.java
##########
@@ -103,7 +104,51 @@ public void shouldReturnOOMError_forWriteOperations_whenThresholdReached()
     fillMemory(jedis, false);
 
     assertThatThrownBy(
-        () -> jedis.set("{key}oneMoreKey", makeLongStringValue(2 * LARGE_VALUE_SIZE)))
+        () -> jedis.set("{key}oneMoreKey", "value"))
+            .hasMessageContaining("OOM");
+
+    memoryPressureThread.interrupt();
+    memoryPressureThread.join();

Review comment:
       Should this `join` use a timeout like `join(GeodeAwaitility.getTimeout().getSeconds(), TimeUnit.SECONDS)`?

##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/OutOfMemoryDUnitTest.java
##########
@@ -103,7 +104,51 @@ public void shouldReturnOOMError_forWriteOperations_whenThresholdReached()
     fillMemory(jedis, false);
 
     assertThatThrownBy(
-        () -> jedis.set("{key}oneMoreKey", makeLongStringValue(2 * LARGE_VALUE_SIZE)))
+        () -> jedis.set("{key}oneMoreKey", "value"))
+            .hasMessageContaining("OOM");
+
+    memoryPressureThread.interrupt();
+    memoryPressureThread.join();
+  }
+
+  @Test
+  public void shouldReturnOOMError_forSubscribe_whenThresholdReached()
+      throws InterruptedException {
+    IgnoredException.addIgnoredException(expectedEx);
+    IgnoredException.addIgnoredException("LowMemoryException");
+    MockSubscriber mockSubscriber = new MockSubscriber();
+    int redisServerPort1 = clusterStartUp.getRedisPort(1);
+    JedisCluster subJedis =
+        new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1), REDIS_CLIENT_TIMEOUT);
+
+    memoryPressureThread = new Thread(makeMemoryPressureRunnable());

Review comment:
       `ExecutorServiceRule` will work well within the controller JVM. If you need a version of the rule that works within any JVM in a dunit test, then use `DistributedExecutorServiceRule`

##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/OutOfMemoryDUnitTest.java
##########
@@ -103,7 +104,51 @@ public void shouldReturnOOMError_forWriteOperations_whenThresholdReached()
     fillMemory(jedis, false);
 
     assertThatThrownBy(
-        () -> jedis.set("{key}oneMoreKey", makeLongStringValue(2 * LARGE_VALUE_SIZE)))
+        () -> jedis.set("{key}oneMoreKey", "value"))
+            .hasMessageContaining("OOM");
+
+    memoryPressureThread.interrupt();
+    memoryPressureThread.join();
+  }
+
+  @Test
+  public void shouldReturnOOMError_forSubscribe_whenThresholdReached()
+      throws InterruptedException {
+    IgnoredException.addIgnoredException(expectedEx);
+    IgnoredException.addIgnoredException("LowMemoryException");
+    MockSubscriber mockSubscriber = new MockSubscriber();
+    int redisServerPort1 = clusterStartUp.getRedisPort(1);
+    JedisCluster subJedis =
+        new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1), REDIS_CLIENT_TIMEOUT);
+
+    memoryPressureThread = new Thread(makeMemoryPressureRunnable());

Review comment:
       You might want to consider using `ExecutorServiceRule` or `DistributedExecutorServiceRule` with a `CompletableFuture` instead of directly using a thread:
   ```
   @Rule
   public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule();
   ```
   ```
       CompletableFuture<Void> memoryPressure = executorServiceRule.runAsync(() -> {
         try {
           while (true) {
             Thread.sleep(1000);
           }
         } catch (InterruptedException e) {
           // done
         }
       });
   
       // do something
       
       memoryPressure.cancel(true);
   ```
   The `cancel(true)` will interrupt the thread. If the test throws before that, `ExecutorServiceRule` will clean up any threads during tear down by invoking shutdownNow which will also interrupt any running threads. If the thread resists interrupt and hangs, the rule should print a stack trace for it as a test failure which would then make it easy to debug.




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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