[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5385: GEODE-8370: Add test for maxInactiveInterval

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

dschneider-pivotal commented on a change in pull request #5385:
URL: https://github.com/apache/geode/pull/5385#discussion_r458216460



##########
File path: geode-redis/src/acceptanceTest/java/session/NativeRedisSessionExpirationAcceptanceTest.java
##########
@@ -50,4 +50,10 @@ public static void setup() {
   public void sessionShouldTimeout_whenAppFailsOverToAnotherRedisServer() {
     // Only using one server for Native Redis
   }
+
+  @Test
+  @Ignore

Review comment:
       Normally when I see an ignore on a test I want to know why the test is being ignored. I think it is something we could fix and then have this test. In this case I think we are overriding a test that we will never do with native redis. It seems like the empty method impl takes care of that and you would not need the ignore. This is just my opinion, I'd be okay if you leave the ignore if you think that is best.

##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/session/SessionExpirationDUnitTest.java
##########
@@ -86,15 +100,78 @@ public void sessionShouldTimeout_whenAppFailsOverToAnotherRedisServer() {
     }
   }
 
+  @Test
+  public void sessionShouldNotTimeout_whenPersisted() {
+    String sessionCookie = createNewSessionWithNote(APP2, "note1");
+    setMaxInactiveInterval(APP2, sessionCookie, -1);
+
+    compareMaxInactiveIntervals();
+  }
+
   private void waitForTheSessionToExpire(String sessionId) {
     GeodeAwaitility.await().ignoreExceptions().atMost((SHORT_SESSION_TIMEOUT + 5), TimeUnit.SECONDS)
         .until(
-            () -> jedisConnetedToServer1.ttl("spring:session:sessions:expires:" + sessionId) < 0);
+            () -> jedisConnetedToServer1.ttl("spring:session:sessions:expires:" + sessionId) == -2);
   }
 
   private void refreshSession(String sessionCookie, int sessionApp) {
     GeodeAwaitility.await()
         .during(SHORT_SESSION_TIMEOUT + 2, TimeUnit.SECONDS)
         .until(() -> getSessionNotes(sessionApp, sessionCookie) != null);
   }
+
+  void setMaxInactiveInterval(int sessionApp, String sessionCookie, int maxInactiveInterval) {
+    HttpHeaders requestHeaders = new HttpHeaders();
+    requestHeaders.add("Cookie", sessionCookie);
+    HttpEntity<Integer> request = new HttpEntity<>(maxInactiveInterval, requestHeaders);
+    new RestTemplate()
+        .postForEntity(
+            "http://localhost:" + ports.get(sessionApp) + "/setMaxInactiveInterval",
+            request,
+            Integer.class)
+        .getHeaders();
+  }
+
+  private void compareMaxInactiveIntervals() {
+    cluster.getVM(1).invoke(() -> {
+      for (int j = 0; j < 113; j++) {

Review comment:
       instead of the hard coded "113", ask the PartitionedRegion for its max bucket. See PartitionedRegion.getTotalNumberOfBuckets. This will help in the future when we allow the redis bucket count to be configured




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