divijvaidya commented on code in PR #14483:
URL: https://github.com/apache/kafka/pull/14483#discussion_r1366804550
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -151,9 +151,8 @@ private Cache<Uuid, Entry> initEmptyCache(long maxSize) {
.weigher((Uuid key, Entry entry) -> {
return (int) entry.entrySizeBytes;
})
- // removeListener is invoked when either the entry is
invalidated (means manual removal by the caller) or
- // evicted (means removal due to the policy)
- .removalListener((Uuid key, Entry entry, RemovalCause cause)
-> {
+ // evictionListener is invoked when RemovalCause.wasEvicted()
is true
Review Comment:
Please update the comment here to explicitly specify whether it would be
executed sync or async with cache eviction. You can say:
This listener is invoked each time an entry is being automatically removed
due to eviction. The cache with invoke this listener during the atomic
operation to remove the entry (refer:
https://github.com/ben-manes/caffeine/wiki/Removal), hence, care must be taken
to ensure that this operation is not expensive.
Note that this listener is not invoked when RemovalCause from cache is
EXPLICIT or REPLACED (e.g. on Cache.invalidate(), Cache.put() etc.) For a
complete list see:
https://www.javadoc.io/static/com.github.ben-manes.caffeine/caffeine/2.9.2/index.html?com/github/benmanes/caffeine/cache/Caffeine.html.
Hence, any operation required after removal from cache must be performed
manually for these scenarios.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -187,7 +186,18 @@ public File cacheDir() {
public void remove(Uuid key) {
lock.readLock().lock();
try {
- internalCache.invalidate(key);
+ internalCache.asMap().computeIfPresent(key, (k, v) -> {
+ try {
+ v.markForCleanup();
+ if (!expiredIndexes.offer(v)) {
+ log.error("Error while inserting entry {} for key {}
into the cleaner queue because queue is full.", v, k);
+ }
+ } catch (IOException e) {
+ throw new KafkaException(e);
+ }
+ // Returning null to remove the key from the cache
+ return null;
+ });
Review Comment:
could be moved into a function since this is used 3 times in the same class
in remove(), in removeAll() and in evictionListener.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
