yifan-c commented on code in PR #262:
URL: https://github.com/apache/cassandra-sidecar/pull/262#discussion_r2383439363
##########
server/src/test/java/org/apache/cassandra/sidecar/acl/RoleAuthorizationsCacheTest.java:
##########
@@ -106,7 +118,11 @@ void testCacheSizeAlwaysOne() throws InterruptedException
// New entries fetched during refreshes
assertThat(cache.getAuthorizations("test_role2").size()).isOne();
+
assertThat(sidecarMetrics.server().cache().rolePermissionsCacheMetrics.snapshot().hitCount()).isZero();
assertThat(cache.getAll().size()).isOne();
+ assertThat(cache.getAuthorizations("test_role2").size()).isOne();
+
assertThat(sidecarMetrics.server().cache().rolePermissionsCacheMetrics.snapshot().hitCount()).isOne();
+
Review Comment:
Can you also assert on the load success count and potentially introduce a
scenario that test the cache miss count?
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java:
##########
@@ -119,13 +123,25 @@ public Map<K, V> getAll()
return Collections.unmodifiableMap(cache.asMap());
}
+ /**
+ * Invalidate a key.
+ * @param k key to invalidate
+ */
+ public void invalidate(K k)
+ {
+ if (cache != null)
+ {
+ cache.invalidate(k);
+ }
+ }
+
private LoadingCache<K, V> initCache()
{
return Caffeine.newBuilder()
- // setting refreshAfterWrite and expireAfterWrite to
same value makes sure no stale
- // data is fetched after expire time
-
.refreshAfterWrite(config.expireAfterAccess().quantity(),
config.expireAfterAccess().unit())
-
.expireAfterWrite(config.expireAfterAccess().quantity(),
config.expireAfterAccess().unit())
+ // The cache keeps the entry until it's last access has
expired. This avoids repeated calls to
+ // db for the same key during high volume requests.
+
.expireAfterAccess(config.expireAfterAccess().quantity(),
config.expireAfterAccess().unit())
Review Comment:
`refreshAfterWrite` ensures the load to the database is capped, i.e _at
most_ once every D duration per entry, which "avoids repeated calls during high
volume requests" already. When the entry is stable, an **async** call is made
to load the data and the stale data is returned while fetch. Current thread is
not blocked.
Meanwhile, `expireAfterAccess` could keep the potentially stale entry in the
cache for longer duration, and even fewer DB access. However, when data is
expired, it is removed from cache immediately. And the subsequent calls to
`get()` will **block** the threads until the new value is loaded. It can
potentially cause thundering herd problem, if multiple threads are getting the
same expired entry.
I would suggest keeping `refreshAfterWrite` only. It gives a good balance
between capping the DB access (load is predictable) and freshness of data.
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java:
##########
@@ -119,13 +123,25 @@ public Map<K, V> getAll()
return Collections.unmodifiableMap(cache.asMap());
}
+ /**
+ * Invalidate a key.
+ * @param k key to invalidate
+ */
+ public void invalidate(K k)
+ {
+ if (cache != null)
+ {
+ cache.invalidate(k);
Review Comment:
Please add a log message when invalidating a key. It should be useful to
confirm the invalidation for debugging purpose. Assuming the frequency should
be low, the log level can be `INFO`.
The method is not currently used.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
