diff --git a/CHANGELOG.md b/CHANGELOG.md index e544b860d027a..2a59e34ba992d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java b/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java index 25c1ba9675600..8b7fe3346803c 100644 --- a/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java +++ b/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 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 fieldMap(String key, String xOpaqueId) { @@ -77,6 +82,34 @@ private static Map 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 cache; + + private KeyDedupeCache() { + cache = CacheBuilder.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(); + } } } diff --git a/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java b/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java index 96ee7831c20ed..bce513a77fc7d 100644 --- a/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java +++ b/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()); + } }