geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dschnei...@apache.org
Subject incubator-geode git commit: GEODE-1718: fix replace on overflowed entry
Date Thu, 04 Aug 2016 17:04:12 GMT
Repository: incubator-geode
Updated Branches:
  refs/heads/develop ccb4fd0df -> 91e234f85


GEODE-1718: fix replace on overflowed entry

When replace checks the current value for equality
with the expected value it now gets it from
the EntryEventImpl which the replace operation
had already called setOldValue on.
Added a unit test that confirms that replace
works on an overflowed entry and also handles
INVALID values correctly.


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/91e234f8
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/91e234f8
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/91e234f8

Branch: refs/heads/develop
Commit: 91e234f85a6f8dfe1eedd59f6978e3667d0fd92a
Parents: ccb4fd0
Author: Darrel Schneider <dschneider@pivotal.io>
Authored: Mon Aug 1 16:12:38 2016 -0700
Committer: Darrel Schneider <dschneider@pivotal.io>
Committed: Thu Aug 4 10:01:14 2016 -0700

----------------------------------------------------------------------
 .../internal/cache/AbstractRegionEntry.java     |  2 +-
 .../internal/cache/AbstractRegionMap.java       | 23 +++--
 .../cache/ReplaceWithOverflowJUnitTest.java     | 94 ++++++++++++++++++++
 3 files changed, 105 insertions(+), 14 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/91e234f8/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java
index 15a5bed..5778a82 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java
@@ -1072,7 +1072,7 @@ public abstract class AbstractRegionEntry implements RegionEntry,
           obj = cdVal;
         }
       }
-      if (obj.getClass().getName().equals(pdx.getClassName())) {
+      if (obj != null && obj.getClass().getName().equals(pdx.getClassName())) {
         GemFireCacheImpl gfc = GemFireCacheImpl.getForPdx("Could not access Pdx registry");
         if (gfc != null) {
           PdxSerializer pdxSerializer;

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/91e234f8/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
index 2e87c2e..226e194 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java
@@ -2844,17 +2844,14 @@ public abstract class AbstractRegionMap implements RegionMap {
     // replace is propagated to server, so no need to check
     // satisfiesOldValue on client
     if (expectedOldValue != null && !replaceOnClient) {
-      ReferenceCountHelper.skipRefCountTracking();
-      
-      @Retained @Released Object v = re._getValueRetain(event.getLocalRegion(), true);
-      
-      ReferenceCountHelper.unskipRefCountTracking();
-      try {
-        if (!AbstractRegionEntry.checkExpectedOldValue(expectedOldValue, v, event.getLocalRegion()))
{
-          return false;
-        }
-      } finally {
-        OffHeapHelper.releaseWithNoTracking(v);
+      assert event.getOperation().guaranteesOldValue();
+      // We already called setOldValueInEvent so the event will have the old value.
+      @Unretained Object v = event.getRawOldValue();
+      // Note that v will be null instead of INVALID because setOldValue
+      // converts INVALID to null.
+      // But checkExpectedOldValue handle this and says INVALID equals null.
+      if (!AbstractRegionEntry.checkExpectedOldValue(expectedOldValue, v, event.getLocalRegion()))
{
+        return false;
       }
     }
     return true;
@@ -2870,7 +2867,7 @@ public abstract class AbstractRegionMap implements RegionMap {
         @Released Object oldValueInVMOrDisk = re.getValueOffHeapOrDiskWithoutFaultIn(event.getLocalRegion());
         ReferenceCountHelper.unskipRefCountTracking();
         try {
-          event.setOldValue(oldValueInVMOrDisk, requireOldValue);
+          event.setOldValue(oldValueInVMOrDisk, needToSetOldValue);
         } finally {
           OffHeapHelper.releaseWithNoTracking(oldValueInVMOrDisk);
         }
@@ -2882,7 +2879,7 @@ public abstract class AbstractRegionMap implements RegionMap {
         
         ReferenceCountHelper.unskipRefCountTracking();
         try {
-          event.setOldValue(oldValueInVM, requireOldValue);
+          event.setOldValue(oldValueInVM, needToSetOldValue);
         } finally {
           OffHeapHelper.releaseWithNoTracking(oldValueInVM);
         }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/91e234f8/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java
new file mode 100644
index 0000000..602d33d
--- /dev/null
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java
@@ -0,0 +1,94 @@
+/*
+ * 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 com.gemstone.gemfire.internal.cache;
+
+import java.util.Properties;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import static org.junit.Assert.assertEquals;
+
+import com.gemstone.gemfire.cache.Cache;
+import com.gemstone.gemfire.cache.CacheFactory;
+import com.gemstone.gemfire.cache.DataPolicy;
+import com.gemstone.gemfire.cache.EvictionAction;
+import com.gemstone.gemfire.cache.EvictionAttributes;
+import com.gemstone.gemfire.cache.PartitionAttributesFactory;
+import com.gemstone.gemfire.cache.Region;
+import com.gemstone.gemfire.test.junit.categories.IntegrationTest;
+
+/**
+ * Test the replace method with an entry that has overflowed to disk.
+ */
+@Category(IntegrationTest.class)
+public class ReplaceWithOverflowJUnitTest {
+
+  private static Cache cache;
+  private Region<String, String> region;
+
+  @BeforeClass
+  public static void setUp() {
+    Properties props = new Properties();
+    props.setProperty("mcast-port", "0");
+    props.setProperty("log-level", "info");
+    cache = new CacheFactory(props).create();
+  }
+  
+  @AfterClass
+  public static void tearDown() {
+    if(cache != null && !cache.isClosed()) {
+      cache.close();
+    }
+  }
+
+  @Before
+  public void createRegion() {
+    region = cache.<String,String>createRegionFactory()
+        .setEvictionAttributes(EvictionAttributes.createLRUEntryAttributes(1, EvictionAction.OVERFLOW_TO_DISK))
+        .setPartitionAttributes(new PartitionAttributesFactory<String, String>().setTotalNumBuckets(1).create())
+        .setDataPolicy(DataPolicy.PARTITION).create("ReplaceWithOverflowJUnitTest");
+  }
+  @After
+  public void destroyRegion() {
+    if (region != null) {
+      region.destroyRegion();
+    }
+  }
+
+  @Test
+  public void testReplaceWithOverflow()  { 
+    region.put("1", "1");
+    region.put("2", "2");
+    assertEquals(true, region.replace("1", "1", "one"));
+  }
+  @Test
+  public void testReplaceWithNullValue() {
+    region.create("3", null);
+    assertEquals(false, region.replace("3", "foobar", "three"));
+    assertEquals(true, region.replace("3", null, "three"));
+    assertEquals(true, region.replace("3", "three", "3"));
+    assertEquals(false, region.replace("3", null, "3"));
+    region.invalidate("3");
+    assertEquals(false, region.replace("3", "foobar", "three"));
+    assertEquals(true, region.replace("3", null, "three"));
+  }
+
+}


Mime
View raw message