Addressed Ankit's comments

Signed-off-by: Peter Alfonsi <[email protected]>
opensearch-project · Apr 23, 2024 · 3164464 · 3164464
1 parent 8b63a72
commit 3164464
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 12 deletions.
diff --git a/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/NoopCacheStatsHolder.java
@@ -13,10 +13,16 @@
 /**
  * A dummy version of CacheStatsHolder, which cache implementations use when FeatureFlags.PLUGGABLE_CACHES is false.
  * Returns all-zero stats when calling getImmutableCacheStatsHolder(). Always returns 0 for count().
+ * A singleton instance is used for memory purposes.
  */
 public class NoopCacheStatsHolder implements CacheStatsHolder {
+    private static final NoopCacheStatsHolder singletonInstance = new NoopCacheStatsHolder();
 
-    public NoopCacheStatsHolder() {}
+    private NoopCacheStatsHolder() {}
+
+    public static NoopCacheStatsHolder getInstance() {
+        return singletonInstance;
+    }
 
     @Override
     public void incrementHits(List<String> dimensionValues) {}

diff --git a/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java b/server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java
@@ -54,7 +54,8 @@ public class OpenSearchOnHeapCache<K, V> implements ICache<K, V>, RemovalListene
     private final List<String> dimensionNames;
     private final ToLongBiFunction<ICacheKey<K>, V> weigher;
 
-    private final Settings settings;
+    // private final Settings settings;
+    private final boolean useNoopStats;
 
     public OpenSearchOnHeapCache(Builder<K, V> builder) {
         CacheBuilder<ICacheKey<K>, V> cacheBuilder = CacheBuilder.<ICacheKey<K>, V>builder()
@@ -66,14 +67,14 @@ public OpenSearchOnHeapCache(Builder<K, V> builder) {
         }
         cache = cacheBuilder.build();
         this.dimensionNames = Objects.requireNonNull(builder.dimensionNames, "Dimension names can't be null");
-        if (useNoopStats(builder.getSettings())) {
-            this.cacheStatsHolder = new NoopCacheStatsHolder();
+        this.useNoopStats = builder.useNoopStats;
+        if (useNoopStats) {
+            this.cacheStatsHolder = NoopCacheStatsHolder.getInstance();
         } else {
             this.cacheStatsHolder = new DefaultCacheStatsHolder(dimensionNames);
         }
         this.removalListener = builder.getRemovalListener();
         this.weigher = builder.getWeigher();
-        this.settings = builder.getSettings();
     }
 
     @Override
@@ -130,7 +131,7 @@ public Iterable<ICacheKey<K>> keys() {
 
     @Override
     public long count() {
-        if (useNoopStats(settings)) {
+        if (useNoopStats) {
             return cache.count();
         } else {
             return cacheStatsHolder.count();
@@ -165,11 +166,6 @@ public void onRemoval(RemovalNotification<ICacheKey<K>, V> notification) {
         }
     }
 
-    private boolean useNoopStats(Settings settings) {
-        // Use noop stats when pluggable caching is off
-        return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings);
-    }
-
     /**
      * Factory to create OpenSearchOnheap cache.
      */
@@ -182,10 +178,10 @@ public <K, V> ICache<K, V> create(CacheConfig<K, V> config, CacheType cacheType,
             Map<String, Setting<?>> settingList = OpenSearchOnHeapCacheSettings.getSettingListForCacheType(cacheType);
             Settings settings = config.getSettings();
             ICacheBuilder<K, V> builder = new Builder<K, V>().setDimensionNames(config.getDimensionNames())
+                .setUseNoopStats(useNoopStats(config.getSettings()))
                 .setMaximumWeightInBytes(((ByteSizeValue) settingList.get(MAXIMUM_SIZE_IN_BYTES_KEY).get(settings)).getBytes())
                 .setExpireAfterAccess(((TimeValue) settingList.get(EXPIRE_AFTER_ACCESS_KEY).get(settings)))
                 .setWeigher(config.getWeigher())
-                .setSettings(config.getSettings())
                 .setRemovalListener(config.getRemovalListener());
             Setting<String> cacheSettingForCacheType = CacheSettings.CACHE_TYPE_STORE_NAME.getConcreteSettingForNamespace(
                 cacheType.getSettingPrefix()
@@ -203,6 +199,11 @@ public <K, V> ICache<K, V> create(CacheConfig<K, V> config, CacheType cacheType,
         public String getCacheName() {
             return NAME;
         }
+
+        private boolean useNoopStats(Settings settings) {
+            // Use noop stats when pluggable caching is off
+            return !FeatureFlags.PLUGGABLE_CACHE_SETTING.get(settings);
+        }
     }
 
     /**
@@ -212,12 +213,18 @@ public String getCacheName() {
      */
     public static class Builder<K, V> extends ICacheBuilder<K, V> {
         private List<String> dimensionNames;
+        private boolean useNoopStats;
 
         public Builder<K, V> setDimensionNames(List<String> dimensionNames) {
             this.dimensionNames = dimensionNames;
             return this;
         }
 
+        public Builder<K, V> setUseNoopStats(boolean useNoopStats) {
+            this.useNoopStats = useNoopStats;
+            return this;
+        }
+
         @Override
         public ICache<K, V> build() {
             return new OpenSearchOnHeapCache<K, V>(this);