This is an automated email from the ASF dual-hosted git repository. agingade pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push: new 4e1c63c GEODE-3869: Fix early eviction. (#1152) 4e1c63c is described below commit 4e1c63ca1eab154f24c9a79757e9600496494224 Author: agingade <aging...@pivotal.io> AuthorDate: Tue Dec 12 14:11:25 2017 -0800 GEODE-3869: Fix early eviction. (#1152) * GEODE-3869: Fix early eviction. While code review found that the refactoring moved "evictionNode.unsetRecentlyUsed()" to "appendEntry()" which gets call by other places (adding entry into the list); this causes the recently used entry to be evicted early. --- .../cache/eviction/AbstractEvictionList.java | 2 - .../cache/eviction/LRUListWithAsyncSorting.java | 1 + .../cache/eviction/LRUListWithSyncSorting.java | 1 + .../cache/eviction/AbstractEvictionListTest.java | 1 - .../eviction/LRUListWithAsyncSortingTest.java | 12 ++--- .../LRUListWithSyncSortingIntegrationTest.java | 3 +- .../RegionEntryEvictionIntegrationTest.java | 63 ++++++++++++++++++++++ 7 files changed, 73 insertions(+), 10 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/AbstractEvictionList.java b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/AbstractEvictionList.java index b9ed858..dc9fc7b 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/AbstractEvictionList.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/AbstractEvictionList.java @@ -116,8 +116,6 @@ abstract class AbstractEvictionList implements EvictionList { return; } - evictionNode.unsetRecentlyUsed(); - if (logger.isTraceEnabled(LogMarker.LRU_CLOCK)) { logger.trace(LogMarker.LRU_CLOCK, LocalizedMessage .create(LocalizedStrings.NewLRUClockHand_ADDING_ANODE_TO_LRU_LIST, evictionNode)); diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSorting.java b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSorting.java index 419b056..a14d363 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSorting.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSorting.java @@ -127,6 +127,7 @@ public class LRUListWithAsyncSorting extends AbstractEvictionList { if (evictionNode.isRecentlyUsed() && evictionAttempts < MAX_EVICTION_ATTEMPTS) { evictionAttempts++; + evictionNode.unsetRecentlyUsed(); appendEntry(evictionNode); continue; } diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSorting.java b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSorting.java index 7f7e9ee..3577894 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSorting.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSorting.java @@ -88,6 +88,7 @@ public class LRUListWithSyncSorting extends AbstractEvictionList { logger.trace(LogMarker.LRU_CLOCK, LocalizedMessage .create(LocalizedStrings.NewLRUClockHand_SKIPPING_RECENTLY_USED_ENTRY, aNode)); } + aNode.unsetRecentlyUsed(); appendEntry(aNode); continue; // keep looking } else { diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/AbstractEvictionListTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/AbstractEvictionListTest.java index d0e072a..f25cc40 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/AbstractEvictionListTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/AbstractEvictionListTest.java @@ -161,7 +161,6 @@ public class AbstractEvictionListTest { EvictionNode node = mock(EvictionNode.class); evictionList.appendEntry(node); - verify(node).unsetRecentlyUsed(); verify(node).setNext(evictionList.tail); verify(node).setPrevious(evictionList.head); assertThat(evictionList.tail.previous()).isSameAs(node); diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSortingTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSortingTest.java index 04ab3e9..e5c1d63 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSortingTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithAsyncSortingTest.java @@ -177,9 +177,9 @@ public class LRUListWithAsyncSortingTest { when(recentlyUsedNode.next()).thenReturn(list.tail); list.incrementRecentlyUsed(); - // unsetRecentlyUsed() is called once on appendEntry(...) and once during scan + // unsetRecentlyUsed() is called once during scan await().atMost(10, TimeUnit.SECONDS) - .until(() -> verify(recentlyUsedNode, times(2)).unsetRecentlyUsed()); + .until(() -> verify(recentlyUsedNode, times(1)).unsetRecentlyUsed()); realExecutor.shutdown(); } @@ -196,9 +196,9 @@ public class LRUListWithAsyncSortingTest { when(recentlyUsedNode.next()).thenReturn(recentlyUsedNode); list.incrementRecentlyUsed(); - // unsetRecentlyUsed() is called once on appendEntry(...) and once during scan + // unsetRecentlyUsed() is called once during scan await().atMost(10, TimeUnit.SECONDS) - .until(() -> verify(recentlyUsedNode, times(2)).unsetRecentlyUsed()); + .until(() -> verify(recentlyUsedNode, times(1)).unsetRecentlyUsed()); realExecutor.shutdown(); } @@ -226,9 +226,9 @@ public class LRUListWithAsyncSortingTest { when(recentlyUsedNode.isRecentlyUsed()).thenReturn(true); list.incrementRecentlyUsed(); - // unsetRecentlyUsed() is called once on appendEntry(...) and once during scan + // unsetRecentlyUsed() is called once during scan await().atMost(10, TimeUnit.SECONDS) - .until(() -> verify(recentlyUsedNode, times(2)).unsetRecentlyUsed()); + .until(() -> verify(recentlyUsedNode, times(1)).unsetRecentlyUsed()); assertThat(list.tail.previous()).isEqualTo(recentlyUsedNode); realExecutor.shutdown(); } diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSortingIntegrationTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSortingIntegrationTest.java index 0bb28b5..384b3fc 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSortingIntegrationTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/LRUListWithSyncSortingIntegrationTest.java @@ -149,7 +149,8 @@ public class LRUListWithSyncSortingIntegrationTest { } @Test - public void evictsRecentlyUsedNodeIfOverLimit() throws Exception { + public void getEvictableEntryReturnsRecentlyUsedNodeAfterSearchMaxEntriesExceeded() + throws Exception { IntStream.range(10, 16).forEach(i -> { EvictionNode node = new LRUTestEntry(i); nodes.add(node); diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/RegionEntryEvictionIntegrationTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/RegionEntryEvictionIntegrationTest.java new file mode 100644 index 0000000..305ab81 --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/eviction/RegionEntryEvictionIntegrationTest.java @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You 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 org.apache.geode.internal.cache.eviction; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; + +import org.apache.geode.cache.Cache; +import org.apache.geode.cache.CacheFactory; +import org.apache.geode.cache.EvictionAttributes; +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.test.junit.categories.IntegrationTest; + +@Category(IntegrationTest.class) +public class RegionEntryEvictionIntegrationTest { + private Region<String, String> region; + + @Rule + public TestName testName = new TestName(); + + @Before + public void setUp() throws Exception { + region = createRegion(); + } + + @Test + public void verifyMostRecentEntryIsNotEvicted() { + region.create("one", "one"); + region.create("twoo", "twoo"); + region.put("one", "one"); + region.put("twoo", "twoo"); + + region.create("threee", "threee"); + assertThat(region.size()).isEqualTo(2); + assertThat(region.containsKey("threee")).isTrue(); + } + + private Region<String, String> createRegion() throws Exception { + Cache cache = new CacheFactory().set("locators", "").set("mcast-port", "0").create(); + return cache.<String, String>createRegionFactory(RegionShortcut.REPLICATE) + .setEvictionAttributes(EvictionAttributes.createLRUEntryAttributes(2)) + .create(testName.getMethodName()); + } + +} -- To stop receiving notification emails like this one, please contact ['"commits@geode.apache.org" <commits@geode.apache.org>'].