geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jasonhu...@apache.org
Subject geode git commit: GEODE-2182: try-catch around size estimation for index look ups
Date Wed, 07 Dec 2016 17:51:59 GMT
Repository: geode
Updated Branches:
  refs/heads/develop ef96dba9a -> c2b1c8b1c


GEODE-2182: try-catch around size estimation for index look ups

* Fix possible race condition where entry is destroyed


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

Branch: refs/heads/develop
Commit: c2b1c8b1c172073824a8e5c48c45a4285f15e6b1
Parents: ef96dba
Author: Jason Huynh <huynhja@gmail.com>
Authored: Mon Dec 5 13:53:22 2016 -0800
Committer: Jason Huynh <huynhja@gmail.com>
Committed: Wed Dec 7 09:28:38 2016 -0800

----------------------------------------------------------------------
 .../query/internal/index/CompactRangeIndex.java |  2 +
 .../query/internal/index/MemoryIndexStore.java  | 12 ++-
 .../functional/IndexOnEntrySetJUnitTest.java    | 89 ++++++++++++++------
 3 files changed, 68 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/geode/blob/c2b1c8b1/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
index a8a38bd..80568f5 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
@@ -437,6 +437,8 @@ public class CompactRangeIndex extends AbstractIndex {
           }
           break;
       }
+    } catch (EntryDestroyedException e) {
+      return Integer.MAX_VALUE;
     } finally {
       updateIndexUseEndStats(start, false);
     }

http://git-wip-us.apache.org/repos/asf/geode/blob/c2b1c8b1/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/MemoryIndexStore.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/MemoryIndexStore.java
b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/MemoryIndexStore.java
index 3635897..e9cd070 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/MemoryIndexStore.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/MemoryIndexStore.java
@@ -247,10 +247,7 @@ public class MemoryIndexStore implements IndexStore {
    * Find the old key by traversing the forward map in case of in-place update modification
If not
    * found it means the value object was modified with same value. So oldKey is same as newKey.
    * 
-   * @param newKey
-   * @param entry
    * @return oldKey
-   * @throws TypeMismatchException
    */
   private Object getOldKey(Object newKey, RegionEntry entry) throws TypeMismatchException
{
     for (Object mapEntry : valueToEntriesMap.entrySet()) {
@@ -406,7 +403,6 @@ public class MemoryIndexStore implements IndexStore {
    * RegionEntry) { return Collections.singleton(regionEntries); } return (Collection)
    * regionEntries; }
    */
-
   @Override
   public CloseableIterator<IndexStoreEntry> get(Object indexKey) {
     return new MemoryIndexStoreIterator(
@@ -574,7 +570,6 @@ public class MemoryIndexStore implements IndexStore {
   /**
    * A bi-directional iterator over the CSL. Iterates over the entries of CSL where entry
is a
    * mapping (value -> Collection) as well as over the Collection.
-   * 
    */
   private class MemoryIndexStoreIterator implements CloseableIterator<IndexStoreEntry>
{
     final Map map;
@@ -662,8 +657,9 @@ public class MemoryIndexStore implements IndexStore {
       }
 
       RegionEntry re = (RegionEntry) valuesIterator.next();
-      if (re == null)
+      if (re == null) {
         throw new NoSuchElementException();
+      }
 
       currentEntry.setMemoryIndexStoreEntry(currKey, re);
       return currentEntry;
@@ -718,7 +714,6 @@ public class MemoryIndexStore implements IndexStore {
 
   /**
    * A wrapper over the entry in the CSL index map. It maps IndexKey -> RegionEntry
-   * 
    */
   class MemoryIndexStoreEntry implements IndexStoreEntry {
     private Object deserializedIndexKey;
@@ -771,6 +766,9 @@ public class MemoryIndexStore implements IndexStore {
     private Object key, value;
 
     public CachedEntryWrapper(LocalRegion.NonTXEntry entry) {
+      if (IndexManager.testHook != null) {
+        IndexManager.testHook.hook(201);
+      }
       this.key = entry.getKey();
       this.value = entry.getValue();
     }

http://git-wip-us.apache.org/repos/asf/geode/blob/c2b1c8b1/geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexOnEntrySetJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexOnEntrySetJUnitTest.java
b/geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexOnEntrySetJUnitTest.java
index 6034d53..f659836 100644
--- a/geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexOnEntrySetJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexOnEntrySetJUnitTest.java
@@ -14,6 +14,10 @@
  */
 package org.apache.geode.cache.query.functional;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
 import org.apache.geode.cache.*;
 import org.apache.geode.cache.query.*;
 import org.apache.geode.cache.query.internal.QueryObserverAdapter;
@@ -91,7 +95,15 @@ public class IndexOnEntrySetJUnitTest {
   public void testQueriesOnReplicatedRegion() throws Exception {
     testRegion = createReplicatedRegion(testRegionName);
     String regionPath = "/" + testRegionName + ".entrySet entry";
-    executeQueryTest(getQueriesOnRegion(testRegionName), "entry.key.Index", regionPath);
+    executeQueryTest(getQueriesOnRegion(testRegionName), "entry.key.Index", regionPath, 200);
+  }
+
+  @Test
+  public void testEntryDestroyedRaceWithSizeEstimateReplicatedRegion() throws Exception {
+    testRegion = createReplicatedRegion(testRegionName);
+    String regionPath = "/" + testRegionName + ".entrySet entry";
+    executeQueryTestDestroyDuringSizeEstimation(getQueriesOnRegion(testRegionName),
+        "entry.key.Index", regionPath, 201);
   }
 
   /**
@@ -102,7 +114,7 @@ public class IndexOnEntrySetJUnitTest {
   public void testQueriesOnPartitionedRegion() throws Exception {
     testRegion = createPartitionedRegion(testRegionName);
     String regionPath = "/" + testRegionName + ".entrySet entry";
-    executeQueryTest(getQueriesOnRegion(testRegionName), "entry.key.Index", regionPath);
+    executeQueryTest(getQueriesOnRegion(testRegionName), "entry.key.Index", regionPath, 200);
   }
 
   private Region createReplicatedRegion(String regionName) throws ParseException {
@@ -140,24 +152,26 @@ public class IndexOnEntrySetJUnitTest {
     }
   }
 
-  /**** Query Execution Helpers ****/
+  /****
+   * Query Execution Helpers
+   ****/
 
-  private void executeQueryTest(String[] queries, String indexedExpression, String regionPath)
-      throws Exception {
+  private void executeQueryTest(String[] queries, String indexedExpression, String regionPath,
+      int testHookSpot) throws Exception {
     Cache cache = CacheUtils.getCache();
     boolean[] booleanVals = {true, false};
     for (String query : queries) {
       for (boolean isDestroy : booleanVals) {
         clearData(testRegion);
         populateRegion(testRegion);
-        Assert.assertNotNull(cache.getRegion(testRegionName));
-        Assert.assertEquals(numElem, cache.getRegion(testRegionName).size());
+        assertNotNull(cache.getRegion(testRegionName));
+        assertEquals(numElem, cache.getRegion(testRegionName).size());
         if (isDestroy) {
           helpTestFunctionalIndexForQuery(query, indexedExpression, regionPath,
-              new DestroyEntryTestHook(testRegion));
+              new DestroyEntryTestHook(testRegion, testHookSpot), 1);
         } else {
           helpTestFunctionalIndexForQuery(query, indexedExpression, regionPath,
-              new InvalidateEntryTestHook(testRegion));
+              new InvalidateEntryTestHook(testRegion, testHookSpot), 1);
         }
       }
     }
@@ -166,22 +180,18 @@ public class IndexOnEntrySetJUnitTest {
     for (String query : queries) {
       clearData(testRegion);
       populateRegion(testRegion);
-      Assert.assertNotNull(cache.getRegion(testRegionName));
-      Assert.assertEquals(numElem, cache.getRegion(testRegionName).size());
+      assertNotNull(cache.getRegion(testRegionName));
+      assertEquals(numElem, cache.getRegion(testRegionName).size());
       helpTestFunctionalIndexForQuery(query, indexedExpression, regionPath,
-          new PutEntryTestHook(testRegion));
+          new PutEntryTestHook(testRegion, testHookSpot), 1);
     }
   }
 
   /**
    * helper method to test against a functional index make sure there is no UNDEFINED result
-   * 
-   * @param query
-   * 
-   * @throws Exception
    */
   private SelectResults helpTestFunctionalIndexForQuery(String query, String indexedExpression,
-      String regionPath, AbstractTestHook testHook) throws Exception {
+      String regionPath, AbstractTestHook testHook, int expectedSize) throws Exception {
     MyQueryObserverAdapter observer = new MyQueryObserverAdapter();
     QueryObserverHolder.setInstance(observer);
     IndexManager.testHook = testHook;
@@ -194,27 +204,40 @@ public class IndexOnEntrySetJUnitTest {
       if (row instanceof Struct) {
         Object[] fields = ((Struct) row).getFieldValues();
         for (Object field : fields) {
-          Assert.assertTrue(field != QueryService.UNDEFINED);
+          assertTrue(field != QueryService.UNDEFINED);
           if (field instanceof String) {
-            Assert.assertTrue(((String) field).compareTo(newValue) != 0);
+            assertTrue(((String) field).compareTo(newValue) != 0);
           }
         }
       } else {
-        Assert.assertTrue(row != QueryService.UNDEFINED);
+        assertTrue(row != QueryService.UNDEFINED);
         if (row instanceof String) {
-          Assert.assertTrue(((String) row).compareTo(newValue) != 0);
+          assertTrue(((String) row).compareTo(newValue) != 0);
         }
       }
     }
-    Assert.assertTrue(indexedResults.size() > 0);
-    Assert.assertTrue(observer.indexUsed);
-    Assert.assertTrue(((AbstractTestHook) IndexManager.testHook).isTestHookCalled());
+    assertTrue(indexedResults.size() >= expectedSize);
+    assertTrue(observer.indexUsed);
+    assertTrue(((AbstractTestHook) IndexManager.testHook).isTestHookCalled());
     ((AbstractTestHook) IndexManager.testHook).reset();
     qs.removeIndex(index);
 
     return indexedResults;
   }
 
+  private void executeQueryTestDestroyDuringSizeEstimation(String[] queries,
+      String indexedExpression, String regionPath, int testHookSpot) throws Exception {
+    Cache cache = CacheUtils.getCache();
+    for (String query : queries) {
+      clearData(testRegion);
+      populateRegion(testRegion);
+      assertNotNull(cache.getRegion(testRegionName));
+      assertEquals(numElem, cache.getRegion(testRegionName).size());
+      helpTestFunctionalIndexForQuery(query, indexedExpression, regionPath,
+          new DestroyEntryTestHook(testRegion, testHookSpot), 0);
+    }
+  }
+
   class MyQueryObserverAdapter extends QueryObserverAdapter {
     public boolean indexUsed = false;
 
@@ -255,6 +278,12 @@ public class IndexOnEntrySetJUnitTest {
     Object waitObj = new Object();
     Region r;
 
+    private int testHookSpot;
+
+    public AbstractTestHook(int testHookSpot) {
+      this.testHookSpot = testHookSpot;
+    }
+
     public void reset() {
       isTestHookCalled = false;
     }
@@ -270,7 +299,7 @@ public class IndexOnEntrySetJUnitTest {
 
     @Override
     public void hook(int spot) throws RuntimeException {
-      if (spot == 200) {
+      if (spot == testHookSpot) {
         if (!isTestHookCalled) {
           isTestHookCalled = true;
           try {
@@ -297,7 +326,9 @@ public class IndexOnEntrySetJUnitTest {
 
   class DestroyEntryTestHook extends AbstractTestHook {
 
-    DestroyEntryTestHook(Region r) {
+    DestroyEntryTestHook(Region r, int testHookSpot) {
+
+      super(testHookSpot);
       this.r = r;
     }
 
@@ -312,7 +343,8 @@ public class IndexOnEntrySetJUnitTest {
 
   class InvalidateEntryTestHook extends AbstractTestHook {
 
-    InvalidateEntryTestHook(Region r) {
+    InvalidateEntryTestHook(Region r, int testHookSpot) {
+      super(testHookSpot);
       this.r = r;
     }
 
@@ -327,7 +359,8 @@ public class IndexOnEntrySetJUnitTest {
 
   class PutEntryTestHook extends AbstractTestHook {
 
-    PutEntryTestHook(Region r) {
+    PutEntryTestHook(Region r, int testHookSpot) {
+      super(testHookSpot);
       this.r = r;
     }
 


Mime
View raw message