[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7359: GEODE-10047: Correct logging to report configured expiration interval

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

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



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/RedisStats.java
##########
@@ -144,24 +144,15 @@ public long getKeyspaceMisses() {
     return keyspaceMisses.get();
   }
 
-  public long startPassiveExpirationCheck() {
-    return getCurrentTimeNanos();
-  }
-
   public void endPassiveExpirationCheck(long start, long expireCount) {
     geodeRedisStats.endPassiveExpirationCheck(start, expireCount);
   }
 
-  public long startExpiration() {

Review comment:
       I would be in favor of keeping these "bookend" start methods. I know that is this particular case all that start does is return getCurrentTimeNanos. But I think it is worth preserving the start/end pattern. And it would make it simple in the future to add other starts that are changed on start (like an in progress stat). I know these particular start methods can be unwrapped but then the abstraction is different and it is not as obvious in the caller how the start and end calls correspond.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/PassiveExpirationManager.java
##########
@@ -76,8 +77,8 @@ private void doDataExpiration(RegionProvider regionProvider) {
       }
     } catch (CacheClosedException ignore) {
     } catch (RuntimeException | Error ex) {
-      logger.warn("Passive expiration failed. Will try again in 1 second.",
-          ex);
+      logger.warn("Passive expiration failed. Will try again in " + passiveExpirationInterval

Review comment:
       Something you don't need to clean up in this pr but that you remind me of is that I got the naming of this expiration manager wrong. Native Redis calls this "active" expiration. The expiration we do when we do an op and find the key expired is what native redis calls "passive" expiration. So these stats and log messages may be a bit confusing to native redis users.




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