CASSSIDECAR-369: Database access during auth flow is blocking event-loop thread #308
Conversation
sarankk
force-pushed
the
CASSSIDECAR-369
branch
2 times, most recently
from
7ef9085 to
9dc3bff
Compare
| public V get(K k) | ||
| public Future<V> get(K k) | ||
| { | ||
| return servicePool.executeBlocking(() -> getCached(k), false); |
There was a problem hiding this 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/AuthCache.java
Outdated
Show resolved
Hide resolved
| 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; | ||
| }); |
There was a problem hiding this comment.
Leverage Future.any to get result sooner without waiting for all futures to complete.
| 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; | |
| }); | |
| 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)); |
There was a problem hiding this comment.
Good idea, updated it.
sarankk
force-pushed
the
CASSSIDECAR-369
branch
from
9dc3bff to
f8c2e05
Compare
| Future<Boolean> isAdminFuture | ||
| = adminIdentityResolver.isAdmin(identity).compose(adminValue -> adminValue | ||
| ? Future.succeededFuture() | ||
| : Future.failedFuture("not admin")); | ||
| // Sidecar recognizes identities in identity_to_role table as authenticated | ||
| Future<Boolean> inCacheFuture | ||
| = identityToRoleCache.containsKey(identity).compose(hasRole -> hasRole | ||
| ? Future.succeededFuture() | ||
| : Future.failedFuture("no role mapped"));; | ||
|
|
||
| Future<Boolean> isValidFuture | ||
| = Future.any(isAdminFuture, inCacheFuture) | ||
| .compose(success -> Future.succeededFuture(true), | ||
| failure -> Future.succeededFuture(false)); |
There was a problem hiding this comment.
Both isAdmin and containsKey triggers the loading function in the IdentityToRoleCache.
The two concurrent futures could lead to the function being evaluated twice, according to the AuthCache.get behavior.
Should we eval isAdmin first, then identityToRoleCache.containsKey? The second does not need to trigger the loading function again, since isAdmin call already either populate the cache or not.
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
One minor issue in CassandraIdentityExtractor. OK with it not being addressed.
2 participants
No description provided.