From 21d74d4d28f32007040ea253419deca29fe3d42c Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Mon, 1 Aug 2022 13:16:59 -0700 Subject: [PATCH 1/7] Added LRUCache and unit tests --- .../ab/internal/DefaultLRUCache.java | 134 +++++++++++++++++ .../com/optimizely/ab/internal/LRUCache.java | 27 ++++ .../ab/internal/DefaultLRUCacheTest.java | 137 ++++++++++++++++++ 3 files changed, 298 insertions(+) create mode 100644 core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java create mode 100644 core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java create mode 100644 core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java new file mode 100644 index 000000000..c1c314631 --- /dev/null +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -0,0 +1,134 @@ +/** + * + * Copyright 2022, Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.optimizely.ab.internal; + +import com.optimizely.ab.annotations.VisibleForTesting; + +import java.util.*; + +public class DefaultLRUCache implements LRUCache { + + private final Object lock = new Object(); + + private Integer maxSize; + + private Long timeoutMillis; + + final LinkedList linkedList = new LinkedList<>(); + + final Map hashMap = new HashMap<>(); + + public DefaultLRUCache() { + this.maxSize = DEFAULT_MAX_SIZE; + this.timeoutMillis = (long) (DEFAULT_TIMEOUT_SECONDS * 1000); + } + + public void setMaxSize(Integer size) { + Integer sizeToSet = size; + if (size < 0 ) { + sizeToSet = DEFAULT_MAX_SIZE; + } + + synchronized (lock) { + if (linkedList.size() > sizeToSet) { + // If cache is bigger than the new maxSize, remove additional items from the end. + int itemsToTrim = linkedList.size() - sizeToSet; + for (int i = 0; i < itemsToTrim; i++) { + ItemWrapper item = linkedList.removeLast(); + hashMap.remove(item.key); + } + } + this.maxSize = sizeToSet; + } + } + + public void setTimeout(Long timeoutSeconds) { + this.timeoutMillis = timeoutSeconds * 1000; + } + + public void save(String key, T value) { + if (maxSize == 0) { + // Cache is disabled when maxSize = 0 + return; + } + synchronized (lock) { + ItemWrapper item = new ItemWrapper(key, value); + + if (hashMap.containsKey(key)) { + ItemWrapper oldItem = hashMap.get(key); + linkedList.remove(oldItem); + linkedList.addFirst(item); + hashMap.replace(key, item); + return; + } + + if (linkedList.size() == maxSize) { + ItemWrapper removedItem = linkedList.removeLast(); + hashMap.remove(removedItem.key); + } + + linkedList.addFirst(item); + hashMap.put(key, item); + } + } + + public T lookup(String key) { + if (maxSize == 0) { + // Cache is disabled when maxSize = 0 + return null; + } + + synchronized (lock) { + if (hashMap.containsKey(key)) { + ItemWrapper item = hashMap.get(key); + Long nowMs = new Date().getTime(); + + // ttl = 0 means items never expire. + if (timeoutMillis == 0 || (nowMs - item.timestamp < timeoutMillis)) { + linkedList.remove(item); + linkedList.addFirst(item); + return item.value; + } + + // If item is stale, remove it. + linkedList.remove(item); + hashMap.remove(item.key); + } + return null; + } + } + + public void reset() { + synchronized (lock) { + linkedList.clear(); + hashMap.clear(); + } + } + + @VisibleForTesting + class ItemWrapper { + public String key; + public T value; + public Long timestamp; + + public ItemWrapper(String key, T value) { + this.key = key; + this.value = value; + this.timestamp = new Date().getTime(); + } + } +} diff --git a/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java new file mode 100644 index 000000000..757e251ed --- /dev/null +++ b/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java @@ -0,0 +1,27 @@ +/** + * + * Copyright 2022, Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.optimizely.ab.internal; + +public interface LRUCache { + int DEFAULT_MAX_SIZE = 10000; + int DEFAULT_TIMEOUT_SECONDS = 600; + void save(String key, T value); + T lookup(String key); + void reset(); + void setMaxSize(Integer size); + void setTimeout(Long ttlSeconds); +} diff --git a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java new file mode 100644 index 000000000..f42c12d7a --- /dev/null +++ b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java @@ -0,0 +1,137 @@ +/** + * + * Copyright 2022, Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.optimizely.ab.internal; + +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +import static org.junit.Assert.*; + +public class DefaultLRUCacheTest { + + @Test + public void createSaveAndLookupOneItem() { + LRUCache cache = new DefaultLRUCache<>(); + assertNull(cache.lookup("key1")); + cache.save("key1", "value1"); + assertEquals("value1", cache.lookup("key1")); + } + + @Test + public void saveAndLookupMultipleItems() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); + + // Lookup should move user1 to top of the list and push down others. + assertEquals("user1", cache.linkedList.get(0).key); + assertEquals("user3", cache.linkedList.get(1).key); + assertEquals("user2", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); + + // Lookup should move user2 to top of the list and push down others. + assertEquals("user2", cache.linkedList.get(0).key); + assertEquals("user1", cache.linkedList.get(1).key); + assertEquals("user3", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); + + // Lookup should move user3 to top of the list and push down others. + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + } + + @Test + public void whenCacheIsDisabled() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + cache.setMaxSize(0); + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + assertNull(cache.lookup("user1")); + assertNull(cache.lookup("user2")); + assertNull(cache.lookup("user3")); + } + + @Test + public void whenItemsExpire() throws InterruptedException { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + cache.setTimeout(1L); + cache.save("user1", Arrays.asList("segment1", "segment2")); + assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); + assertEquals(1, cache.linkedList.size()); + assertEquals(1, cache.hashMap.size()); + Thread.sleep(1000); + assertNull(cache.lookup("user1")); + assertEquals(0, cache.linkedList.size()); + assertEquals(0, cache.hashMap.size()); + } + + @Test + public void whenCacheReachesMaxSize() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + cache.setMaxSize(2); + + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + assertEquals(2, cache.linkedList.size()); + assertEquals(2, cache.hashMap.size()); + + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); + assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); + assertNull(cache.lookup("user1")); + } + + @Test + public void whenMaxSizeIsReducedInBetween() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); + assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); + + assertEquals(3, cache.linkedList.size()); + assertEquals(3, cache.hashMap.size()); + + cache.setMaxSize(1); + + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); + assertNull(cache.lookup("user1")); + assertNull(cache.lookup("user2")); + + assertEquals(1, cache.linkedList.size()); + assertEquals(1, cache.hashMap.size()); + } +} From 8fbd39897eb31d1de7661e5aba15ca1372744645 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Wed, 3 Aug 2022 10:50:02 -0700 Subject: [PATCH 2/7] Added peek --- .../ab/internal/DefaultLRUCache.java | 13 +++++++ .../com/optimizely/ab/internal/LRUCache.java | 3 +- .../ab/internal/DefaultLRUCacheTest.java | 34 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java index c1c314631..4b488d401 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -112,6 +112,19 @@ public T lookup(String key) { } } + public T peek(String key) { + if (maxSize == 0) { + // Cache is disabled when maxSize = 0 + return null; + } + + if (hashMap.containsKey(key)) { + return hashMap.get(key).value; + } + + return null; + } + public void reset() { synchronized (lock) { linkedList.clear(); diff --git a/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java index 757e251ed..44bb51225 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java @@ -21,7 +21,8 @@ public interface LRUCache { int DEFAULT_TIMEOUT_SECONDS = 600; void save(String key, T value); T lookup(String key); + T peek(String key); void reset(); void setMaxSize(Integer size); - void setTimeout(Long ttlSeconds); + void setTimeout(Long timeoutSeconds); } diff --git a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java index f42c12d7a..b2ea72956 100644 --- a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java +++ b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java @@ -134,4 +134,38 @@ public void whenMaxSizeIsReducedInBetween() { assertEquals(1, cache.linkedList.size()); assertEquals(1, cache.hashMap.size()); } + + @Test + public void saveAndPeekItems() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment1", "segment2"), cache.peek("user1")); + + // Peek should not alter the order of array + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment3", "segment4"), cache.peek("user2")); + + // Peek should not alter the order of array + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + + assertEquals(Arrays.asList("segment5", "segment6"), cache.peek("user3")); + + // Peek should not alter the order of array + assertEquals("user3", cache.linkedList.get(0).key); + assertEquals("user2", cache.linkedList.get(1).key); + assertEquals("user1", cache.linkedList.get(2).key); + } } From 46f00040e7bba20b99b348709055ee65ecb94e09 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Thu, 4 Aug 2022 18:28:06 -0700 Subject: [PATCH 3/7] Changed the implementation to use LinkedHashMap --- .../ab/internal/{LRUCache.java => Cache.java} | 3 +- .../ab/internal/DefaultLRUCache.java | 95 +++++++------------ .../ab/internal/DefaultLRUCacheTest.java | 95 +++++++++---------- 3 files changed, 78 insertions(+), 115 deletions(-) rename core-api/src/main/java/com/optimizely/ab/internal/{LRUCache.java => Cache.java} (94%) diff --git a/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/Cache.java similarity index 94% rename from core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java rename to core-api/src/main/java/com/optimizely/ab/internal/Cache.java index 44bb51225..525ea7d6f 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/LRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/Cache.java @@ -16,12 +16,11 @@ */ package com.optimizely.ab.internal; -public interface LRUCache { +public interface Cache { int DEFAULT_MAX_SIZE = 10000; int DEFAULT_TIMEOUT_SECONDS = 600; void save(String key, T value); T lookup(String key); - T peek(String key); void reset(); void setMaxSize(Integer size); void setTimeout(Long timeoutSeconds); diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java index 4b488d401..b734c4930 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -17,20 +17,28 @@ package com.optimizely.ab.internal; import com.optimizely.ab.annotations.VisibleForTesting; +import com.optimizely.ab.config.parser.DefaultConfigParser; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.*; -public class DefaultLRUCache implements LRUCache { +public class DefaultLRUCache implements Cache { + + private static final Logger logger = LoggerFactory.getLogger(DefaultLRUCache.class); private final Object lock = new Object(); private Integer maxSize; private Long timeoutMillis; - - final LinkedList linkedList = new LinkedList<>(); - - final Map hashMap = new HashMap<>(); + @VisibleForTesting + final LinkedHashMap linkedHashMap = new LinkedHashMap(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return this.size() > maxSize; + } + }; public DefaultLRUCache() { this.maxSize = DEFAULT_MAX_SIZE; @@ -38,21 +46,18 @@ public DefaultLRUCache() { } public void setMaxSize(Integer size) { - Integer sizeToSet = size; - if (size < 0 ) { - sizeToSet = DEFAULT_MAX_SIZE; - } - - synchronized (lock) { - if (linkedList.size() > sizeToSet) { - // If cache is bigger than the new maxSize, remove additional items from the end. - int itemsToTrim = linkedList.size() - sizeToSet; - for (int i = 0; i < itemsToTrim; i++) { - ItemWrapper item = linkedList.removeLast(); - hashMap.remove(item.key); - } + if (linkedHashMap.size() > 0) { + if (size >= linkedHashMap.size()) { + maxSize = size; + } else { + logger.warn("Cannot set max cache size less than current size."); + } + } else { + Integer sizeToSet = size; + if (size < 0) { + sizeToSet = 0; } - this.maxSize = sizeToSet; + maxSize = sizeToSet; } } @@ -65,24 +70,9 @@ public void save(String key, T value) { // Cache is disabled when maxSize = 0 return; } - synchronized (lock) { - ItemWrapper item = new ItemWrapper(key, value); - - if (hashMap.containsKey(key)) { - ItemWrapper oldItem = hashMap.get(key); - linkedList.remove(oldItem); - linkedList.addFirst(item); - hashMap.replace(key, item); - return; - } - if (linkedList.size() == maxSize) { - ItemWrapper removedItem = linkedList.removeLast(); - hashMap.remove(removedItem.key); - } - - linkedList.addFirst(item); - hashMap.put(key, item); + synchronized (lock) { + linkedHashMap.put(key, new ItemWrapper(value)); } } @@ -93,53 +83,32 @@ public T lookup(String key) { } synchronized (lock) { - if (hashMap.containsKey(key)) { - ItemWrapper item = hashMap.get(key); + if (linkedHashMap.containsKey(key)) { + ItemWrapper item = linkedHashMap.get(key); Long nowMs = new Date().getTime(); // ttl = 0 means items never expire. if (timeoutMillis == 0 || (nowMs - item.timestamp < timeoutMillis)) { - linkedList.remove(item); - linkedList.addFirst(item); return item.value; } - // If item is stale, remove it. - linkedList.remove(item); - hashMap.remove(item.key); + linkedHashMap.remove(key); } return null; } } - public T peek(String key) { - if (maxSize == 0) { - // Cache is disabled when maxSize = 0 - return null; - } - - if (hashMap.containsKey(key)) { - return hashMap.get(key).value; - } - - return null; - } - public void reset() { synchronized (lock) { - linkedList.clear(); - hashMap.clear(); + linkedHashMap.clear(); } } - @VisibleForTesting - class ItemWrapper { - public String key; + private class ItemWrapper { public T value; public Long timestamp; - public ItemWrapper(String key, T value) { - this.key = key; + public ItemWrapper(T value) { this.value = value; this.timestamp = new Date().getTime(); } diff --git a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java index b2ea72956..474cb65db 100644 --- a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java +++ b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java @@ -16,18 +16,24 @@ */ package com.optimizely.ab.internal; +import ch.qos.logback.classic.Level; +import org.junit.Rule; import org.junit.Test; import java.util.Arrays; +import java.util.Collection; import java.util.List; +import java.util.Set; import static org.junit.Assert.*; public class DefaultLRUCacheTest { + @Rule + public LogbackVerifier logbackVerifier = new LogbackVerifier(); @Test public void createSaveAndLookupOneItem() { - LRUCache cache = new DefaultLRUCache<>(); + Cache cache = new DefaultLRUCache<>(); assertNull(cache.lookup("key1")); cache.save("key1", "value1"); assertEquals("value1", cache.lookup("key1")); @@ -41,30 +47,34 @@ public void saveAndLookupMultipleItems() { cache.save("user2", Arrays.asList("segment3", "segment4")); cache.save("user3", Arrays.asList("segment5", "segment6")); - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); + String[] itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + assertEquals("user1", itemKeys[0]); + assertEquals("user2", itemKeys[1]); + assertEquals("user3", itemKeys[2]); assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); - // Lookup should move user1 to top of the list and push down others. - assertEquals("user1", cache.linkedList.get(0).key); - assertEquals("user3", cache.linkedList.get(1).key); - assertEquals("user2", cache.linkedList.get(2).key); + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // Lookup should move user1 to bottom of the list and push up others. + assertEquals("user2", itemKeys[0]); + assertEquals("user3", itemKeys[1]); + assertEquals("user1", itemKeys[2]); assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); - // Lookup should move user2 to top of the list and push down others. - assertEquals("user2", cache.linkedList.get(0).key); - assertEquals("user1", cache.linkedList.get(1).key); - assertEquals("user3", cache.linkedList.get(2).key); + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // Lookup should move user2 to bottom of the list and push up others. + assertEquals("user3", itemKeys[0]); + assertEquals("user1", itemKeys[1]); + assertEquals("user2", itemKeys[2]); assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - // Lookup should move user3 to top of the list and push down others. - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // Lookup should move user3 to bottom of the list and push up others. + assertEquals("user1", itemKeys[0]); + assertEquals("user2", itemKeys[1]); + assertEquals("user3", itemKeys[2]); } @Test @@ -86,12 +96,10 @@ public void whenItemsExpire() throws InterruptedException { cache.setTimeout(1L); cache.save("user1", Arrays.asList("segment1", "segment2")); assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); - assertEquals(1, cache.linkedList.size()); - assertEquals(1, cache.hashMap.size()); + assertEquals(1, cache.linkedHashMap.size()); Thread.sleep(1000); assertNull(cache.lookup("user1")); - assertEquals(0, cache.linkedList.size()); - assertEquals(0, cache.hashMap.size()); + assertEquals(0, cache.linkedHashMap.size()); } @Test @@ -103,8 +111,7 @@ public void whenCacheReachesMaxSize() { cache.save("user2", Arrays.asList("segment3", "segment4")); cache.save("user3", Arrays.asList("segment5", "segment6")); - assertEquals(2, cache.linkedList.size()); - assertEquals(2, cache.hashMap.size()); + assertEquals(2, cache.linkedHashMap.size()); assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); @@ -122,50 +129,38 @@ public void whenMaxSizeIsReducedInBetween() { assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - assertEquals(3, cache.linkedList.size()); - assertEquals(3, cache.hashMap.size()); + assertEquals(3, cache.linkedHashMap.size()); cache.setMaxSize(1); + logbackVerifier.expectMessage(Level.WARN, "Cannot set max cache size less than current size."); + + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); + assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - assertNull(cache.lookup("user1")); - assertNull(cache.lookup("user2")); - assertEquals(1, cache.linkedList.size()); - assertEquals(1, cache.hashMap.size()); + assertEquals(3, cache.linkedHashMap.size()); } @Test - public void saveAndPeekItems() { + public void whenCacheIsReset() { DefaultLRUCache> cache = new DefaultLRUCache<>(); - cache.save("user1", Arrays.asList("segment1", "segment2")); cache.save("user2", Arrays.asList("segment3", "segment4")); cache.save("user3", Arrays.asList("segment5", "segment6")); - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); - - assertEquals(Arrays.asList("segment1", "segment2"), cache.peek("user1")); - - // Peek should not alter the order of array - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); + assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); + assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); + assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - assertEquals(Arrays.asList("segment3", "segment4"), cache.peek("user2")); + assertEquals(3, cache.linkedHashMap.size()); - // Peek should not alter the order of array - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); + cache.reset(); - assertEquals(Arrays.asList("segment5", "segment6"), cache.peek("user3")); + assertNull(cache.lookup("user1")); + assertNull(cache.lookup("user2")); + assertNull(cache.lookup("user3")); - // Peek should not alter the order of array - assertEquals("user3", cache.linkedList.get(0).key); - assertEquals("user2", cache.linkedList.get(1).key); - assertEquals("user1", cache.linkedList.get(2).key); + assertEquals(0, cache.linkedHashMap.size()); } } From 3fda8cb7c0bd67d762bdf3bd245bed1fb35194f3 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Thu, 4 Aug 2022 18:34:44 -0700 Subject: [PATCH 4/7] added a unit test to make sure `save` also renews the item --- .../ab/internal/DefaultLRUCacheTest.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java index 474cb65db..446abca70 100644 --- a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java +++ b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java @@ -77,6 +77,44 @@ public void saveAndLookupMultipleItems() { assertEquals("user3", itemKeys[2]); } + @Test + public void saveShouldReorderList() { + DefaultLRUCache> cache = new DefaultLRUCache<>(); + + cache.save("user1", Arrays.asList("segment1", "segment2")); + cache.save("user2", Arrays.asList("segment3", "segment4")); + cache.save("user3", Arrays.asList("segment5", "segment6")); + + String[] itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + assertEquals("user1", itemKeys[0]); + assertEquals("user2", itemKeys[1]); + assertEquals("user3", itemKeys[2]); + + cache.save("user1", Arrays.asList("segment1", "segment2")); + + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // save should move user1 to bottom of the list and push up others. + assertEquals("user2", itemKeys[0]); + assertEquals("user3", itemKeys[1]); + assertEquals("user1", itemKeys[2]); + + cache.save("user2", Arrays.asList("segment3", "segment4")); + + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // save should move user2 to bottom of the list and push up others. + assertEquals("user3", itemKeys[0]); + assertEquals("user1", itemKeys[1]); + assertEquals("user2", itemKeys[2]); + + cache.save("user3", Arrays.asList("segment5", "segment6")); + + itemKeys = cache.linkedHashMap.keySet().toArray(new String[0]); + // save should move user3 to bottom of the list and push up others. + assertEquals("user1", itemKeys[0]); + assertEquals("user2", itemKeys[1]); + assertEquals("user3", itemKeys[2]); + } + @Test public void whenCacheIsDisabled() { DefaultLRUCache> cache = new DefaultLRUCache<>(); From 1763ac21ff9dc91474d862616f67a0a7dcc71627 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Thu, 4 Aug 2022 18:42:13 -0700 Subject: [PATCH 5/7] Added a line break for better formatting --- .../main/java/com/optimizely/ab/internal/DefaultLRUCache.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java index b734c4930..90817a115 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -32,6 +32,7 @@ public class DefaultLRUCache implements Cache { private Integer maxSize; private Long timeoutMillis; + @VisibleForTesting final LinkedHashMap linkedHashMap = new LinkedHashMap(16, 0.75f, true) { @Override From 1611a8691a82aef255509cccd65d76a6a0a5a0f1 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Fri, 5 Aug 2022 10:54:44 -0700 Subject: [PATCH 6/7] incorporated review feedback --- .../com/optimizely/ab/internal/Cache.java | 2 - .../ab/internal/DefaultLRUCache.java | 51 ++++++------------- .../ab/internal/DefaultLRUCacheTest.java | 40 ++------------- 3 files changed, 19 insertions(+), 74 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/internal/Cache.java b/core-api/src/main/java/com/optimizely/ab/internal/Cache.java index 525ea7d6f..ba667ebd2 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/Cache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/Cache.java @@ -22,6 +22,4 @@ public interface Cache { void save(String key, T value); T lookup(String key); void reset(); - void setMaxSize(Integer size); - void setTimeout(Long timeoutSeconds); } diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java index 90817a115..d87e01be1 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -17,53 +17,32 @@ package com.optimizely.ab.internal; import com.optimizely.ab.annotations.VisibleForTesting; -import com.optimizely.ab.config.parser.DefaultConfigParser; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.util.*; public class DefaultLRUCache implements Cache { - private static final Logger logger = LoggerFactory.getLogger(DefaultLRUCache.class); - private final Object lock = new Object(); - private Integer maxSize; + private final Integer maxSize; - private Long timeoutMillis; + private final Long timeoutMillis; @VisibleForTesting - final LinkedHashMap linkedHashMap = new LinkedHashMap(16, 0.75f, true) { + final LinkedHashMap linkedHashMap = new LinkedHashMap(16, 0.75f, true) { @Override - protected boolean removeEldestEntry(Map.Entry eldest) { + protected boolean removeEldestEntry(Map.Entry eldest) { return this.size() > maxSize; } }; public DefaultLRUCache() { - this.maxSize = DEFAULT_MAX_SIZE; - this.timeoutMillis = (long) (DEFAULT_TIMEOUT_SECONDS * 1000); - } - - public void setMaxSize(Integer size) { - if (linkedHashMap.size() > 0) { - if (size >= linkedHashMap.size()) { - maxSize = size; - } else { - logger.warn("Cannot set max cache size less than current size."); - } - } else { - Integer sizeToSet = size; - if (size < 0) { - sizeToSet = 0; - } - maxSize = sizeToSet; - } + this(DEFAULT_MAX_SIZE, DEFAULT_TIMEOUT_SECONDS); } - public void setTimeout(Long timeoutSeconds) { - this.timeoutMillis = timeoutSeconds * 1000; + public DefaultLRUCache(Integer maxSize, Integer timeoutSeconds) { + this.maxSize = maxSize < 0 ? 0 : maxSize; + this.timeoutMillis = (timeoutSeconds < 0) ? 0 : (timeoutSeconds * 1000L); } public void save(String key, T value) { @@ -73,7 +52,7 @@ public void save(String key, T value) { } synchronized (lock) { - linkedHashMap.put(key, new ItemWrapper(value)); + linkedHashMap.put(key, new CacheEntity(value)); } } @@ -85,12 +64,12 @@ public T lookup(String key) { synchronized (lock) { if (linkedHashMap.containsKey(key)) { - ItemWrapper item = linkedHashMap.get(key); + CacheEntity entity = linkedHashMap.get(key); Long nowMs = new Date().getTime(); - // ttl = 0 means items never expire. - if (timeoutMillis == 0 || (nowMs - item.timestamp < timeoutMillis)) { - return item.value; + // ttl = 0 means entities never expire. + if (timeoutMillis == 0 || (nowMs - entity.timestamp < timeoutMillis)) { + return entity.value; } linkedHashMap.remove(key); @@ -105,11 +84,11 @@ public void reset() { } } - private class ItemWrapper { + private class CacheEntity { public T value; public Long timestamp; - public ItemWrapper(T value) { + public CacheEntity(T value) { this.value = value; this.timestamp = new Date().getTime(); } diff --git a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java index 446abca70..79aa96ff3 100644 --- a/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java +++ b/core-api/src/test/java/com/optimizely/ab/internal/DefaultLRUCacheTest.java @@ -16,20 +16,14 @@ */ package com.optimizely.ab.internal; -import ch.qos.logback.classic.Level; -import org.junit.Rule; import org.junit.Test; import java.util.Arrays; -import java.util.Collection; import java.util.List; -import java.util.Set; import static org.junit.Assert.*; public class DefaultLRUCacheTest { - @Rule - public LogbackVerifier logbackVerifier = new LogbackVerifier(); @Test public void createSaveAndLookupOneItem() { @@ -117,8 +111,8 @@ public void saveShouldReorderList() { @Test public void whenCacheIsDisabled() { - DefaultLRUCache> cache = new DefaultLRUCache<>(); - cache.setMaxSize(0); + DefaultLRUCache> cache = new DefaultLRUCache<>(0,Cache.DEFAULT_TIMEOUT_SECONDS); + cache.save("user1", Arrays.asList("segment1", "segment2")); cache.save("user2", Arrays.asList("segment3", "segment4")); cache.save("user3", Arrays.asList("segment5", "segment6")); @@ -130,8 +124,7 @@ public void whenCacheIsDisabled() { @Test public void whenItemsExpire() throws InterruptedException { - DefaultLRUCache> cache = new DefaultLRUCache<>(); - cache.setTimeout(1L); + DefaultLRUCache> cache = new DefaultLRUCache<>(Cache.DEFAULT_MAX_SIZE, 1); cache.save("user1", Arrays.asList("segment1", "segment2")); assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); assertEquals(1, cache.linkedHashMap.size()); @@ -142,8 +135,7 @@ public void whenItemsExpire() throws InterruptedException { @Test public void whenCacheReachesMaxSize() { - DefaultLRUCache> cache = new DefaultLRUCache<>(); - cache.setMaxSize(2); + DefaultLRUCache> cache = new DefaultLRUCache<>(2, Cache.DEFAULT_TIMEOUT_SECONDS); cache.save("user1", Arrays.asList("segment1", "segment2")); cache.save("user2", Arrays.asList("segment3", "segment4")); @@ -156,30 +148,6 @@ public void whenCacheReachesMaxSize() { assertNull(cache.lookup("user1")); } - @Test - public void whenMaxSizeIsReducedInBetween() { - DefaultLRUCache> cache = new DefaultLRUCache<>(); - cache.save("user1", Arrays.asList("segment1", "segment2")); - cache.save("user2", Arrays.asList("segment3", "segment4")); - cache.save("user3", Arrays.asList("segment5", "segment6")); - - assertEquals(Arrays.asList("segment1", "segment2"), cache.lookup("user1")); - assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); - assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - - assertEquals(3, cache.linkedHashMap.size()); - - cache.setMaxSize(1); - - logbackVerifier.expectMessage(Level.WARN, "Cannot set max cache size less than current size."); - - assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - assertEquals(Arrays.asList("segment3", "segment4"), cache.lookup("user2")); - assertEquals(Arrays.asList("segment5", "segment6"), cache.lookup("user3")); - - assertEquals(3, cache.linkedHashMap.size()); - } - @Test public void whenCacheIsReset() { DefaultLRUCache> cache = new DefaultLRUCache<>(); From c63affde3d66255d158c5e32429d9efce1f49c95 Mon Sep 17 00:00:00 2001 From: zashraf1985 Date: Fri, 5 Aug 2022 11:15:33 -0700 Subject: [PATCH 7/7] Fixed a spotBugs issue --- .../main/java/com/optimizely/ab/internal/DefaultLRUCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java index d87e01be1..a531c5c83 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java @@ -41,7 +41,7 @@ public DefaultLRUCache() { } public DefaultLRUCache(Integer maxSize, Integer timeoutSeconds) { - this.maxSize = maxSize < 0 ? 0 : maxSize; + this.maxSize = maxSize < 0 ? Integer.valueOf(0) : maxSize; this.timeoutMillis = (timeoutSeconds < 0) ? 0 : (timeoutSeconds * 1000L); }