yifan-c commented on code in PR #308:
URL: https://github.com/apache/cassandra-sidecar/pull/308#discussion_r2685151815
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java:
##########
@@ -92,16 +95,28 @@ protected LoadingCache<K, V> cache()
}
/**
- * Retrieves a value from the cache. Will call {@link
LoadingCache#get(Object)} which will
+ * Retrieves a value from the cache asynchronously. Will call {@link
LoadingCache#get(Object)} which will
* "load" the value if it's not present, thus populating the key. When the
cache is disabled, data is fetched
- * with loadFunction.
+ * with loadFunction. The cache retrieval is offloaded to a worker thread
to avoid blocking the event loop.
*
* @param k key
- * @return The current value of {@code K} if cached or loaded.
+ * @return A {@link Future} containing the current value of {@code K} if
cached or loaded.
* <p>
* See {@link LoadingCache#get(Object)} for possible exceptions.
*/
- public V get(K k)
+ public Future<V> get(K k)
+ {
+ return servicePool.executeBlocking(() -> getCached(k), false);
+ }
+
+ /**
+ * Helper method to retrieve value from cache synchronously. Used by
{@link #get(K)} which offloads
+ * this to a worker thread.
+ *
+ * @param k key
+ * @return The current value of {@code K} if cached or loaded
Review Comment:
nit: use lower case `the`. Because the sentence here is `@return the current
...`. It is common to use lower case letters following `@return`, `@param`,
etc. in javadoc.
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/AuthCache.java:
##########
@@ -92,16 +95,28 @@ protected LoadingCache<K, V> cache()
}
/**
- * Retrieves a value from the cache. Will call {@link
LoadingCache#get(Object)} which will
+ * Retrieves a value from the cache asynchronously. Will call {@link
LoadingCache#get(Object)} which will
* "load" the value if it's not present, thus populating the key. When the
cache is disabled, data is fetched
- * with loadFunction.
+ * with loadFunction. The cache retrieval is offloaded to a worker thread
to avoid blocking the event loop.
*
* @param k key
- * @return The current value of {@code K} if cached or loaded.
+ * @return A {@link Future} containing the current value of {@code K} if
cached or loaded.
* <p>
* See {@link LoadingCache#get(Object)} for possible exceptions.
*/
- public V get(K k)
+ public Future<V> get(K k)
+ {
+ return servicePool.executeBlocking(() -> getCached(k), false);
Review Comment:
Unconditionally dispatching the execution to a service pool could hurt
performance. In most cases, we expect cache hit and it should return
immediately, instead of incurring context switch.
You can do a non-blocking check by using `getIfPresent()` and only schedule
`getCached` on cache missing. Note that it still permits concurrent calls for a
small time window. It is a good balance between code simplicity and
performance.
I would not suggest to save the `Future`, as it could lead to wrong thread
continuation, if not used carefully.
Please update all the other similar use cases that unconditionally executes
in servicePool.
##########
server/src/main/java/org/apache/cassandra/sidecar/acl/authorization/CachedAuthorizationHandler.java:
##########
@@ -264,15 +266,27 @@ protected void
checkOrFetchAuthorizations(Promise<Boolean> promise, Authorizatio
promise.fail(new HttpException(FORBIDDEN.code()));
}
- protected boolean isAdmin(List<String> identities)
+ protected Future<Boolean> isAdmin(List<String> identities)
{
+ if (identities.isEmpty())
+ {
+ return Future.succeededFuture(false);
+ }
+ List<Future<Boolean>> adminFutures = new
ArrayList<>(identities.size());
for (String identity : identities)
{
- if (adminIdentityResolver.isAdmin(identity))
- {
- return true;
- }
+ adminFutures.add(adminIdentityResolver.isAdmin(identity));
}
- return false;
+ return Future.all(adminFutures)
+ .map(cf -> {
+ for (int i = 0; i < cf.size(); i++)
+ {
+ if (Boolean.TRUE.equals(cf.resultAt(i)))
+ {
+ return true;
+ }
+ }
+ return false;
+ });
Review Comment:
Leverage `Future.any` to get result sooner without waiting for all futures
to complete.
```suggestion
for (String identity : identities)
{
adminFutures.add(adminIdentityResolver.isAdmin(identity)
.compose(isAdmin ->
isAdmin ? Future.succeededFuture() : Future.failedFuture("Not admin")));
}
return Future.any(adminFutures)
.compose(success -> Future.succeededFuture(true),
failure -> Future.succeededFuture(false));
```
--
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]
