[GitHub] [couchdb] jaydoane commented on a change in pull request #3588: Aegis improvements

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

jaydoane commented on a change in pull request #3588:
URL: https://github.com/apache/couchdb/pull/3588#discussion_r642638120



##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -286,8 +278,10 @@ is_key_fresh(UUID) ->
     Now = fabric2_util:now(sec),
 
     case ets:lookup(?KEY_CHECK, UUID) of
-        [{UUID, ExpiresAt}] when ExpiresAt >= Now -> true;
-        _ -> false
+        [{UUID, ExpiresAt}] when ExpiresAt - ?KEY_EARLY_EXPIRE_SEC > Now ->

Review comment:
       > this is virtually the same as reducing [aegis] cache_max_age_sec config parameter on 5 sec less
   
   I don't think so, because `ExpiresAt` is being compared to `now()` in both `is_key_fresh` and also `remove_expired_entries`, whereas this change basically says that the key is not "fresh" if it's 5 seconds away from expiring.  I think the `badmatch` crash exposed this race condition, and `KEY_EARLY_EXPIRE_SEC` is an attempt to minimize the impact of that race by not trying to use a cached key that's too close to expiring. I wonder if 10 seconds would be better though since that's the frequency used by the expiration reaper?
   
   > Did I really forget to do the same thing here? :(
   
   I think so, but that is effectively what adding this parameter does: `is_key_fresh` returns false not only when the key is expired, but also when it is stale. Maybe a better name would be `KEY_STALE_SEC`?




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