Bound the size of cache in deprecation logger

The current implementation of the map used to de-duplicate deprecation log
messages can grow without bound. This replaces the ConcurrentHashMap
implementation with an `o.o.common.cache.Cache` implementation configured for a
fixed maximum size. To reduce memory overhead, this also changes to storing only
a hash of the key instead of the full text since we never need to retrieve the
original entry.

Signed-off-by: Andrew Ross <[email protected]>

CHANGELOG.md

@@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix `doc_values` only (`index:false`) IP field searching for masks ([#16628](https://github.com/opensearch-project/OpenSearch/pull/16628))
 - Fix stale cluster state custom file deletion ([#16670](https://github.com/opensearch-project/OpenSearch/pull/16670))
 - [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560))
+- Bound the size of cache in deprecation logger ([16702](https://github.com/opensearch-project/OpenSearch/issues/16702))
 
 ### Security
 

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

@@ -32,12 +32,12 @@
 
 package org.opensearch.common.logging;
 
+import org.opensearch.common.cache.Cache;
+import org.opensearch.common.cache.CacheBuilder;
 import org.opensearch.common.collect.MapBuilder;
 import org.opensearch.core.common.Strings;
 
 import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * A logger message used by {@link DeprecationLogger}.
@@ -47,12 +47,17 @@
  */
 public class DeprecatedMessage extends OpenSearchLogMessage {
     public static final String X_OPAQUE_ID_FIELD_NAME = "x-opaque-id";
-    private static final Set<String> keys = ConcurrentHashMap.newKeySet();
+
+    // Arbitrary maximum size, should be much larger than unique number of
+    // loggers, but small relative to heap size.
+    static final int MAX_DEDUPE_CACHE_ENTRIES = 65_536;
+
+    private static final KeyDedupeCache keyDedupeCache = new KeyDedupeCache();
     private final String keyWithXOpaqueId;
 
     public DeprecatedMessage(String key, String xOpaqueId, String messagePattern, Object... args) {
         super(fieldMap(key, xOpaqueId), messagePattern, args);
-        this.keyWithXOpaqueId = new StringBuilder().append(key).append(xOpaqueId).toString();
+        this.keyWithXOpaqueId = key + xOpaqueId;
     }
 
     /**
@@ -62,7 +67,7 @@ public DeprecatedMessage(String key, String xOpaqueId, String messagePattern, Ob
      * Otherwise, a warning can be logged by some test and the upcoming test can be impacted by it.
      */
     public static void resetDeprecatedMessageForTests() {
-        keys.clear();
+        keyDedupeCache.clear();
     }
 
     private static Map<String, Object> fieldMap(String key, String xOpaqueId) {
@@ -77,6 +82,34 @@ private static Map<String, Object> fieldMap(String key, String xOpaqueId) {
     }
 
     public boolean isAlreadyLogged() {
-        return !keys.add(keyWithXOpaqueId);
+        return !keyDedupeCache.add(keyWithXOpaqueId);
+    }
+
+    private static class KeyDedupeCache {
+        private static final Object VALUE = new Object();
+        private final Cache<Integer, Object> cache;
+
+        private KeyDedupeCache() {
+            cache = CacheBuilder.<Integer, Object>builder()
+                .weigher((s, o) -> 1) // Entries are a fixed size (integers)
+                .setMaximumWeight(MAX_DEDUPE_CACHE_ENTRIES)
+                .build();
+        }
+
+        boolean add(String key) {
+            int hash = key.hashCode();
+            // This read-then-write pattern is not actually thread safe. However, the worst case
+            // of 'n' racing threads here is that a warning will be logged 'n' times instead
+            // of just once. I believe this is better than adding more synchronization overhead.
+            if (cache.get(hash) == null) {
+                cache.put(hash, VALUE);
+                return true;
+            }
+            return false;
+        }
+
+        void clear() {
+            cache.invalidateAll();
+        }
     }
 }

server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java

@@ -69,4 +69,26 @@ public void testDuplicateLogMessages() {
         // assert that only unique warnings are logged
         assertWarnings("Deprecated message 1", "Deprecated message 2", "Deprecated message 3");
     }
+
+    public void testMaximumSizeOfCache() {
+        final int maxEntries = DeprecatedMessage.MAX_DEDUPE_CACHE_ENTRIES;
+        // Fill up the cache, asserting every message is new
+        for (int i = 0; i < maxEntries; i++) {
+            DeprecatedMessage message = new DeprecatedMessage("key-" + i, "message-" + i, "");
+            assertFalse(message.toString(), message.isAlreadyLogged());
+        }
+        // Do the same thing except assert every message has been seen
+        for (int i = 0; i < maxEntries; i++) {
+            DeprecatedMessage message = new DeprecatedMessage("key-" + i, "message-" + i, "");
+            assertTrue(message.toString(), message.isAlreadyLogged());
+        }
+        // Add one more new entry, asserting it is new
+        DeprecatedMessage message = new DeprecatedMessage("key-new", "message-new", "");
+        assertFalse(message.toString(), message.isAlreadyLogged());
+        assertTrue(message.toString(), message.isAlreadyLogged());
+        // Check the first entry added, asserting it has been evicted and is now seen as new
+        DeprecatedMessage message2 = new DeprecatedMessage("key-0", "message-0", "");
+        assertFalse(message2.toString(), message2.isAlreadyLogged());
+        assertTrue(message2.toString(), message2.isAlreadyLogged());
+    }
 }