Expiring cache #2331

jaydoane · 2019-11-25T07:52:36Z

Overview

This implements an FDB backed key value cache, where each entry has a "stale" and "expires" time associated with it. Once the current time exceeds the "expires" time, the entry is automatically removed.

Potentially useful for implementing e.g. interfaces to external systems such as OAuth 2.

Testing recommendations

make check apps=couch_expiring_cache

Related Issues or Pull Requests

Checklist

Code is written and works correctly
Changes are covered by tests
Any new configurable parameters are documented in rel/overlay/etc/default.ini
A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

src/couch_expiring_cache/README.md

src/couch_expiring_cache/src/couch_expiring_cache_fdb.erl

src/couch_expiring_cache/src/couch_expiring_cache.erl

nickva · 2019-11-25T23:03:17Z

@jaydoane nice works, small and easy to understand!

Made a few comments in-line.

Was also wondering if you thought about building it more as a library, such that it would plug into to another application's supervisor with something like:

couch_expiring_cache:start_link(Name, Options) when is_atom(Name) ->
    gen_server:start_link({local, Name}, ...) -> ets:new(Name, [named_table, ...]).

Then we wouldn't need a way to disable or enable the whole application and it would be started only as needed. What do you think? If you'd rather keep it as is, that'll work just. Just thought to explore another option.

garrensmith

Nice work. Some minor feedback.

garrensmith · 2019-11-26T17:45:42Z

src/couch_expiring_cache/src/couch_expiring_cache.erl

+
+-spec lookup(Name :: binary(), Key :: binary()) ->
+ not_found | {fresh, Val :: binary()} | {stale, Val :: binary()} | expired.
+lookup(Name, Key) ->


I like the idea that @nickva mentions above about adding an optional transaction. I think it should also apply to the lookup.

b939216 enables optional transactions for both insert and lookup operations.

garrensmith · 2019-11-26T17:47:20Z

src/couch_expiring_cache/src/couch_expiring_cache_fdb.erl

+]).
+
+
+-define(XC, 53). % coordinate with fabric2.hrl


I'm a bit nervous that we have it here instead of fabric2.hrl, it could be easy for someone else to later use the namespace for something else. Also XC isn't a particluarly clean name. I have no idea what it means. Could you expand it a little.

I agree that it's not a great solution. I just copied this from couch_jobs, but maybe we should just make it a practice to directly modify fabric2.hrl as necessary?

Also, expanded XC to EXPIRING_CACHE in 7144ceb

garrensmith · 2019-11-26T17:53:01Z

src/couch_expiring_cache/test/couch_expiring_cache_tests.erl

+
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("eunit/include/eunit.hrl").


Any chance you could make this one of the new shiny exunit tests instead?

I would love nothing more than to use elixir here, but the expiring cache has no HTTP interface, so unfortunately it would be extremely inconvenient :(

Its possible to do that now https://github.com/apache/couchdb/tree/prototype/fdb-layer/test/elixir#how-to-write-exunit-tests

You don't need an http interface to use the exunit tests.

jaydoane · 2019-12-02T06:49:04Z

Was also wondering if you thought about building it more as a library, such that it would plug into to another application's supervisor something like:

couch_expiring_cache:start_link(Name, Options) when is_atom(Name) ->
    gen_server:start_link({local, Name}, ...) -> ets:new(Name, [named_table, ...]).

@nickva this is an interesting idea, and I hadn't considered adding the cache to an existing supervisor tree, as your snippet seems to imply. I'm guessing that Options could be a map containing optional overrides for period, jitter, and batch size. I think this would also imply a subtle change to the data model, where each Name gets its own gen_server on each node, and

(?EXPIRING_CACHE, ?EXP, ExpiresTS, Name, Key) := ()
would become
(?EXPIRING_CACHE, ?EXP, Name, ExpiresTS, Key) := ()

The main drawback I could think of with such an approach is that unexpired entries of abandoned/changed Names. would never be cleared without some explicit garbage collection mechanism. Alternately, it could use a system like couch_jobs, and explicitly manage names, but that adds complexity.

nickva · 2019-12-05T19:20:54Z

@jaydoane I was thinking it would be like having N expiring caches, one for each application that needs it. Each would have its own timeouts, etc. Presumably the expiring cache for OAuth2 vs some other auth mechanism could need different expiry and stale timeouts. The model would then look like:

(?EXPIRING_CACHE, Name, ?PK, Key) := (Val, StaleTS, ExpireTS)
(?EXPIRING_CACHE, Name, ?EXP, ExpireTS, Key) := ()

So there would be one for OAuth2 with something like Name = oauth2_xcache. If there another auth library application it would have Name = otherlib_xcache. Each application would then be in charge of starting its own expiry cache child in its supervisor, cleaning up stale entries, updating entries, etc.

jaydoane · 2019-12-05T19:38:07Z

(?EXPIRING_CACHE, Name, ?PK, Key) := (Val, StaleTS, ExpireTS)
(?EXPIRING_CACHE, Name, ?EXP, ExpireTS, Key) := ()

@nickva I think the main issue with this schema is that if application Name is retired, or changes its name while there are still unexpired cache entries, those entries will remain as garbage indefinitely in FDB without a way to get cleaned up. We could of course create a registry for cache names similar to how job types are registered, but that adds complexity. Not sure if there's a simpler way to do it, or whether it's even a real concern. I suppose we could just create a cleanup function that should be called (manually) when a Name is retired/changed?

EDIT: It occurs to me that it's possible to clear all keys under the (?EXPIRING_CACHE) prefix, which is sort of the "nuclear option" for key cleanup, but since this is just a cache, it seems like a relatively benign way to ensure any garbage from abandoned Names gets cleared. In short, I agree that overall this change is a substantial improvement, and an update is in the works!

src/couch_expiring_cache/src/couch_expiring_cache_server.erl

nickva · 2019-12-10T23:28:35Z

src/couch_expiring_cache/src/couch_expiring_cache_server.erl

+ timer_ref => schedule_remove_expired(Period, MaxJitter),
+ oldest_ts => 0,
+ elapsed => 0,
+ largest_elapsed => 0,


What does largest_elapsed track? Is it a stat for debugging?

Yeah, since we don't currently have a way to collect metrics, it's just a simple way to see how long it's taking (in the worst case) to delete expired entries. If it gets close to 5 seconds, we might need to decrease the period, and/or lower the batch size.

nickva · 2019-12-10T23:35:20Z

src/couch_expiring_cache/src/couch_expiring_cache.erl

+-spec insert(Tx :: jtx(), Name :: binary(), Key :: binary(), Value :: binary(),
+ StaleTS :: millisecond(), ExpiresTS :: millisecond()) -> ok.
+insert(Tx, Name, Key, Value, StaleTS, ExpiresTS) ->
+ couch_jobs_fdb:tx(couch_jobs_fdb:get_jtx(Tx), fun(JTx) ->


If you want, maybe add some guards on the types of arguments.

is_binary/1 for Name, Key and Value and is_integer/1 for StaleTs and ExpiresTS

nickva · 2019-12-10T23:38:58Z

rel/overlay/etc/default.ini

+
+[expiring_cache]
+;
+; How often to remove expired entries
+; period_msec = 5000
+;
+; Jitter applied when removing expired entries
+; max_jitter_msec = 1000
+;
+; Maximum number of entries removed in a single transaction
+; batch_size = 1000


Since it's a library, let's skip updating the default.ini changes. Each application can then have its own section

Argh, I forgot to remove this when I converted to a library. Done.

nickva · 2019-12-11T00:04:44Z

@jaydoane Looks great. See a few minor notes.

To run tests I had to add it to the subdirs in rebar:

--- a/rebar.config.script
+++ b/rebar.config.script
@@ -92,6 +92,7 @@ SubDirs = [
     "src/fabric",
     "src/couch_jobs",
+    "src/couch_expiring_cache",

Otherwise tests ran and passed.

Created an expiring cache manually and saw that entries were being returned as fresh, stale, expired then cleaned up and returned as non_found.

 couch_expiring_cache:insert(<<"auth-decision">>, <<"a">>, <<"x">>, erlang:system_time(millisecond) + 6000, erlang:system_time(millisecond) + 10000), couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{fresh,<<"x">>}
([email protected])79> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).                        {fresh,<<"x">>}                                                                                  couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{fresh,<<"x">>}
([email protected])81> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{fresh,<<"x">>}
([email protected])89> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])90> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])91> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])92> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])93> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])94> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])103> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])104> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
{stale,<<"x">>}
([email protected])105> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
expired
([email protected])106> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
expired
([email protected])107> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
not_found
([email protected])109> couch_expiring_cache:lookup(<<"auth-decision">>, <<"a">>).
not_found

Nice work!

+1 (after the minor fixes from the comments)

This is a library for creating an FDB backed key value cache, where each entry has a `stale` and `expires` time associated with it. Once the current time exceeds the `expires` time, the entry is automatically removed. The `stale` time can be used to indicate that a refresh is necessary, while still returning a non-expired value. It is potentially useful for implementing e.g. caches to external systems of record, such as OAuth 2.

jaydoane requested a review from nickva November 25, 2019 07:52

nickva reviewed Nov 25, 2019

View reviewed changes

src/couch_expiring_cache/README.md Outdated Show resolved Hide resolved

nickva reviewed Nov 25, 2019

View reviewed changes

src/couch_expiring_cache/src/couch_expiring_cache_fdb.erl Outdated Show resolved Hide resolved

nickva reviewed Nov 25, 2019

View reviewed changes

src/couch_expiring_cache/src/couch_expiring_cache_fdb.erl Outdated Show resolved Hide resolved

nickva reviewed Nov 25, 2019

View reviewed changes

src/couch_expiring_cache/src/couch_expiring_cache.erl Outdated Show resolved Hide resolved

garrensmith reviewed Nov 26, 2019

View reviewed changes

nickva reviewed Dec 10, 2019

View reviewed changes

src/couch_expiring_cache/src/couch_expiring_cache_server.erl Outdated Show resolved Hide resolved

nickva reviewed Dec 10, 2019

View reviewed changes

jaydoane force-pushed the expiring-cache branch from 515e8a3 to 2f7957c Compare December 11, 2019 00:49

jaydoane merged commit b3899c4 into apache:prototype/fdb-layer Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expiring cache #2331

Expiring cache #2331

jaydoane commented Nov 25, 2019

nickva commented Nov 25, 2019

garrensmith left a comment

garrensmith Nov 26, 2019

jaydoane Dec 2, 2019

garrensmith Nov 26, 2019

jaydoane Dec 2, 2019

garrensmith Nov 26, 2019

jaydoane Dec 2, 2019

garrensmith Dec 2, 2019 •

edited

Loading

jaydoane commented Dec 2, 2019

nickva commented Dec 5, 2019

jaydoane commented Dec 5, 2019 •

edited

Loading

nickva Dec 10, 2019

jaydoane Dec 11, 2019 •

edited

Loading

nickva Dec 10, 2019

jaydoane Dec 11, 2019

nickva Dec 10, 2019

jaydoane Dec 11, 2019

nickva commented Dec 11, 2019

Expiring cache #2331

Expiring cache #2331

Conversation

jaydoane commented Nov 25, 2019

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

nickva commented Nov 25, 2019

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrensmith Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

jaydoane commented Dec 2, 2019

nickva commented Dec 5, 2019

jaydoane commented Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

jaydoane Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickva commented Dec 11, 2019

garrensmith Dec 2, 2019 •

edited

Loading

jaydoane commented Dec 5, 2019 •

edited

Loading

jaydoane Dec 11, 2019 •

edited

Loading