Addressing comments

Signed-off-by: Sagar Upadhyaya <sagar.upadhyaya.121@gmail.com>
opensearch-project · May 6, 2024 · e1eee1b · e1eee1b
1 parent a53e2be
commit e1eee1b
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 27 deletions.
diff --git a/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java b/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java
@@ -54,7 +54,8 @@ public final class ShardRequestCache {
     final CounterMetric missCount = new CounterMetric();
 
     public RequestCacheStats stats() {
-        return new RequestCacheStats(totalMetric.count(), evictionsMetric.count(), hitCount.count(), missCount.count());
+        return new RequestCacheStats(Math.max(0, totalMetric.count()), evictionsMetric.count(), hitCount.count(),
+            missCount.count());
     }
 
     public void onHit() {
@@ -78,15 +79,15 @@ public void onRemoval(long keyRamBytesUsed, BytesReference value, boolean evicte
         if (value != null) {
             dec += value.ramBytesUsed();
         }
-        if ((totalMetric.count() - dec) < 0) {
+        totalMetric.dec(dec);
+        if (totalMetric.count() < 0) {
+            totalMetric.inc(dec);
             logger.warn(
                 "Ignoring the operation to deduct memory: {} from RequestStats memory_size metric as it will "
                     + "go negative. Current memory: {}. This is a bug.",
                 dec,
                 totalMetric.count()
             );
-        } else {
-            totalMetric.dec(dec);
         }
     }
 
@@ -96,25 +97,6 @@ public void onCached(Accountable key, BytesReference value) {
     }
 
     public void onRemoval(Accountable key, BytesReference value, boolean evicted) {
-        if (evicted) {
-            evictionsMetric.inc();
-        }
-        long dec = 0;
-        if (key != null) {
-            dec += key.ramBytesUsed();
-        }
-        if (value != null) {
-            dec += value.ramBytesUsed();
-        }
-        if ((totalMetric.count() - dec) < 0) {
-            logger.warn(
-                "Ignoring the operation to deduct memory: {} from RequestStats memory_size metric as it will "
-                    + "go negative. Current memory: {}. This is a bug.",
-                dec,
-                totalMetric.count()
-            );
-        } else {
-            totalMetric.dec(dec);
-        }
+       onRemoval(key.ramBytesUsed(), value, evicted);
     }
 }
diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java
@@ -244,7 +244,7 @@ public void onRemoval(RemovalNotification<ICacheKey<Key>, BytesReference> notifi
         if (indexShardCacheEntity != null) {
             // Here we match the hashcode to avoid scenario where we deduct stats of older IndexShard(with same
             // shardId) from current IndexShard.
-            if (key.indexShardHashCode == indexShardCacheEntity.getCacheIdentity().hashCode()) {
+            if (key.indexShardHashCode == System.identityHashCode(indexShardCacheEntity.getCacheIdentity())) {
                 indexShardCacheEntity.onRemoval(notification);
             }
         }
@@ -281,7 +281,7 @@ BytesReference getOrCompute(
         String readerCacheKeyId = delegatingCacheHelper.getDelegatingCacheKey().getId();
         assert readerCacheKeyId != null;
         IndexShard indexShard = ((IndexShard) cacheEntity.getCacheIdentity());
-        final Key key = new Key(indexShard.shardId(), cacheKey, readerCacheKeyId, indexShard.hashCode());
+        final Key key = new Key(indexShard.shardId(), cacheKey, readerCacheKeyId, System.identityHashCode(indexShard));
         Loader cacheLoader = new Loader(cacheEntity, loader);
         BytesReference value = cache.computeIfAbsent(getICacheKey(key), cacheLoader);
         if (cacheLoader.isLoaded()) {
@@ -315,7 +315,8 @@ void invalidate(IndicesService.IndexShardCacheEntity cacheEntity, DirectoryReade
             readerCacheKeyId = ((OpenSearchDirectoryReader.DelegatingCacheHelper) cacheHelper).getDelegatingCacheKey().getId();
         }
         IndexShard indexShard = (IndexShard) cacheEntity.getCacheIdentity();
-        cache.invalidate(getICacheKey(new Key(indexShard.shardId(), cacheKey, readerCacheKeyId, indexShard.hashCode())));
+        cache.invalidate(getICacheKey(new Key(indexShard.shardId(), cacheKey, readerCacheKeyId,
+            System.identityHashCode(indexShard))));
     }
 
     /**

diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java
@@ -1212,6 +1212,7 @@ public void testSerializationDeserializationOfCacheKey() throws Exception {
         assertEquals(readerCacheKeyId, key2.readerCacheKeyId);
         assertEquals(((IndexShard) shardCacheEntity.getCacheIdentity()).shardId(), key2.shardId);
         assertEquals(getTermBytes(), key2.value);
+        assertEquals(indexShard.hashCode(), key2.indexShardHashCode);
     }
 
     public void testGetOrComputeConcurrentlyWithMultipleIndices() throws Exception {