Fix LRU comparison algorithm (#18624)

hazelcast/src/main/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparator.java

@@ -33,9 +33,10 @@ public class LRUEvictionPolicyComparator
 
     @Override
     public int compare(EvictableEntryView e1, EvictableEntryView e2) {
-        int result = Long.compare(e1.getLastAccessTime(), e2.getLastAccessTime());
-        // if access times are same, we try to select oldest entry to evict
-        return result == 0 ? Long.compare(e1.getCreationTime(), e2.getCreationTime()) : result;
+        long time1 = Math.max(e1.getCreationTime(), e1.getLastAccessTime());
+        long time2 = Math.max(e2.getCreationTime(), e2.getLastAccessTime());
+
+        return Long.compare(time1, time2);
     }
 
     @Override

hazelcast/src/main/java/com/hazelcast/map/impl/recordstore/AbstractRecordStore.java

@@ -142,6 +142,7 @@ public Record createRecord(Object value, long ttlMillis, long maxIdle, long now)
         Record record = recordFactory.newRecord(value);
         record.setCreationTime(now);
         record.setLastUpdateTime(now);
+        record.setLastAccessTime(now);
 
         updateStatsOnPut(false, now);
         return record;

hazelcast/src/test/java/com/hazelcast/internal/eviction/EvictionPolicyEvaluatorTest.java

@@ -125,20 +125,25 @@ public EvictionPolicyComparator getComparator() {
                 = new ArrayList<EvictionCandidate<Integer, CacheObjectRecord>>();
 
         long baseTime = System.currentTimeMillis();
-
+        long minCreationTime = -1;
         for (int i = 0; i < recordCount; i++) {
             long creationTime = baseTime + (i * 100);
+            minCreationTime = minCreationTime == -1 ? creationTime : Math.min(creationTime, minCreationTime);
+
             CacheObjectRecord record = new CacheObjectRecord(i, creationTime, Long.MAX_VALUE);
             if (i == expectedEvictedRecordValue) {
-                // The record in the middle will be minimum access time.
-                // So, it will be selected for eviction
-                record.setLastAccessTime(baseTime - 1000);
+                // The record in the middle will be minimum access
+                // time. So, it will be selected for eviction
+                // (set creation-time also to keep this condition
+                // true --> creation-time <= last-access-time)
+                record.setCreationTime(minCreationTime);
+                record.setLastAccessTime(minCreationTime + 1);
             } else if (i == expectedExpiredRecordValue) {
                 record.setExpirationTime(System.currentTimeMillis());
             } else {
                 record.setLastAccessTime(creationTime + 1000);
             }
-            records.add(new SimpleEvictionCandidate<Integer, CacheObjectRecord>(i, record));
+            records.add(new SimpleEvictionCandidate<>(i, record));
         }
 
         sleepAtLeastMillis(1);

hazelcast/src/test/java/com/hazelcast/internal/eviction/impl/comparator/LRUEvictionPolicyComparatorTest.java

@@ -0,0 +1,149 @@
+/*
+ * Copyright (c) 2008-2021, Hazelcast, Inc. All Rights Reserved.
+ *
+ * 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.hazelcast.internal.eviction.impl.comparator;
+
+import com.hazelcast.spi.eviction.EvictableEntryView;
+import com.hazelcast.test.HazelcastSerialClassRunner;
+import com.hazelcast.test.annotation.QuickTest;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(HazelcastSerialClassRunner.class)
+@Category(QuickTest.class)
+public class LRUEvictionPolicyComparatorTest {
+
+    private static final long NOW = 20;
+
+    @Test
+    public void lru_comparator_does_not_prematurely_select_newly_created_entries() {
+        // 0. Entries to sort
+        List<TestEntryView> givenEntries = new LinkedList<>();
+        givenEntries.add(new TestEntryView(1, 1, 0));
+        givenEntries.add(new TestEntryView(2, 2, 3));
+        givenEntries.add(new TestEntryView(3, 2, 0));
+        givenEntries.add(new TestEntryView(4, 4, 4));
+        givenEntries.add(new TestEntryView(5, 5, 20));
+        givenEntries.add(new TestEntryView(6, 6, 6));
+        givenEntries.add(new TestEntryView(7, 7, 0));
+        givenEntries.add(new TestEntryView(8, 9, 15));
+        givenEntries.add(new TestEntryView(9, 10, 10));
+        givenEntries.add(new TestEntryView(10, 10, 0));
+
+        // 1. Create expected list of ordered elements by
+        // sorting entries based on their idle-times. Longest
+        // idle time must be the first element of the list.
+        List<TestEntryView> descOrderByIdleTimes = new LinkedList<>(givenEntries);
+        Collections.sort(descOrderByIdleTimes, (o1, o2) -> -Long.compare(idleTime(o1), idleTime(o2)));
+
+        // 2. Then sort given entries by using LRU eviction comparator.
+        Collections.sort(givenEntries, (o1, o2) -> LRUEvictionPolicyComparator.INSTANCE.compare(o1, o2));
+
+        // 3. Check both lists are equal
+        assertEquals(descOrderByIdleTimes, givenEntries);
+    }
+
+    private static long idleTime(EvictableEntryView entryView) {
+        return NOW - Math.max(entryView.getCreationTime(), entryView.getLastAccessTime());
+    }
+
+    private static class TestEntryView implements EvictableEntryView {
+        private long id;
+        private long creationTime;
+        private long lastAccessTime;
+
+        TestEntryView(long id, long creationTime, long lastAccessTime) {
+            this.id = id;
+            this.creationTime = creationTime;
+            this.lastAccessTime = lastAccessTime;
+        }
+
+        @Override
+        public long getCreationTime() {
+            return creationTime;
+        }
+
+        @Override
+        public long getLastAccessTime() {
+            return lastAccessTime;
+        }
+
+        public long getId() {
+            return id;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+
+            TestEntryView that = (TestEntryView) o;
+
+            if (id != that.id) {
+                return false;
+            }
+            if (creationTime != that.creationTime) {
+                return false;
+            }
+            return lastAccessTime == that.lastAccessTime;
+        }
+
+        @Override
+        public int hashCode() {
+            int result = (int) (id ^ (id >>> 32));
+            result = 31 * result + (int) (creationTime ^ (creationTime >>> 32));
+            result = 31 * result + (int) (lastAccessTime ^ (lastAccessTime >>> 32));
+            return result;
+        }
+
+        @Override
+        public String toString() {
+            return "TestEntryView{"
+                    + "id=" + id
+                    + ", now=" + NOW
+                    + ", creationTime=" + creationTime
+                    + ", lastAccessTime=" + lastAccessTime
+                    + ", idleTime=" + idleTime(this) + '}';
+        }
+
+        @Override
+        public Object getKey() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public Object getValue() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public long getHits() {
+            throw new UnsupportedOperationException();
+        }
+    }
+}

hazelcast/src/test/java/com/hazelcast/map/ExpirationTimeTest.java

@@ -344,10 +344,8 @@ public void testExpirationTime_whenMaxIdleTime_isSmallerThan_TTL() {
         map.put(1, 1, 100, TimeUnit.SECONDS);
 
         EntryView<Integer, Integer> entryView = map.getEntryView(1);
-        long lastAccessTime = entryView.getLastAccessTime();
-        long delayToExpiration = lastAccessTime + TimeUnit.SECONDS.toMillis(10);
+        long delayToExpiration = TimeUnit.SECONDS.toMillis(10);
 
-        // lastAccessTime is zero after put, we can find expiration by this calculation
         long expectedExpirationTime = delayToExpiration + entryView.getCreationTime();
         assertEquals(expectedExpirationTime, entryView.getExpirationTime());
     }
@@ -367,14 +365,14 @@ public void testExpirationTime_whenMaxIdleTime_isBiggerThan_TTL() {
     }
 
     @Test
-    public void testLastAccessTime_isZero_afterFirstPut() {
+    public void testLastAccessTime_equals_creationTime_afterFirstPut() {
         IMap<Integer, Integer> map = createMap();
 
         map.put(1, 1);
 
         EntryView<Integer, Integer> entryView = map.getEntryView(1);
 
-        assertEquals(0L, entryView.getLastAccessTime());
+        assertEquals(entryView.getCreationTime(), entryView.getLastAccessTime());
     }
 
     @Test
@@ -438,9 +436,9 @@ public void last_access_time_updated_on_primary_when_read_backup_data_enabled()
 
         long lastAccessTimeBefore = map.getEntryView(1).getLastAccessTime();
 
-        sleepAtLeastMillis(10);
+        sleepAtLeastMillis(500);
         map.get(1);
-        sleepAtLeastMillis(10);
+        sleepAtLeastMillis(500);
         map.get(1);
 
         long lastAccessTimeAfter = map.getEntryView(1).getLastAccessTime();