Skip to content

Commit

Permalink
GEODE-2182: try-catch around size estimation for index look ups
Browse files Browse the repository at this point in the history
* Fix possible race condition where entry is destroyed
  • Loading branch information
jhuynh1 committed Dec 7, 2016
1 parent ef96dba commit c2b1c8b
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,8 @@ public int getSizeEstimate(Object key, int operator, int matchLevel)
}
break;
}
} catch (EntryDestroyedException e) {
return Integer.MAX_VALUE;
} finally {
updateIndexUseEndStats(start, false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,7 @@ public void updateMapping(Object newKey, Object oldKey, RegionEntry entry, Objec
* 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()) {
Expand Down Expand Up @@ -406,7 +403,6 @@ private Object convertToIndexKey(Object key, RegionEntry entry) throws TypeMisma
* RegionEntry) { return Collections.singleton(regionEntries); } return (Collection)
* regionEntries; }
*/

@Override
public CloseableIterator<IndexStoreEntry> get(Object indexKey) {
return new MemoryIndexStoreIterator(
Expand Down Expand Up @@ -574,7 +570,6 @@ public int size() {
/**
* 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;
Expand Down Expand Up @@ -662,8 +657,9 @@ public MemoryIndexStoreEntry next() {
}

RegionEntry re = (RegionEntry) valuesIterator.next();
if (re == null)
if (re == null) {
throw new NoSuchElementException();
}

currentEntry.setMemoryIndexStoreEntry(currKey, re);
return currentEntry;
Expand Down Expand Up @@ -718,7 +714,6 @@ public String printAll() {

/**
* A wrapper over the entry in the CSL index map. It maps IndexKey -> RegionEntry
*
*/
class MemoryIndexStoreEntry implements IndexStoreEntry {
private Object deserializedIndexKey;
Expand Down Expand Up @@ -771,6 +766,9 @@ class CachedEntryWrapper {
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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -91,7 +95,15 @@ private String[] getQueriesOnRegionForPut(String regionName) {
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);
}

/**
Expand All @@ -102,7 +114,7 @@ public void testQueriesOnReplicatedRegion() throws Exception {
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 {
Expand Down Expand Up @@ -140,24 +152,26 @@ private void clearData(Region region) {
}
}

/**** 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);
}
}
}
Expand All @@ -166,22 +180,18 @@ private void executeQueryTest(String[] queries, String indexedExpression, String
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;
Expand All @@ -194,27 +204,40 @@ private SelectResults helpTestFunctionalIndexForQuery(String query, String index
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;

Expand Down Expand Up @@ -255,6 +278,12 @@ abstract class AbstractTestHook implements IndexManager.TestHook {
Object waitObj = new Object();
Region r;

private int testHookSpot;

public AbstractTestHook(int testHookSpot) {
this.testHookSpot = testHookSpot;
}

public void reset() {
isTestHookCalled = false;
}
Expand All @@ -270,7 +299,7 @@ public boolean isTestHookCalled() {

@Override
public void hook(int spot) throws RuntimeException {
if (spot == 200) {
if (spot == testHookSpot) {
if (!isTestHookCalled) {
isTestHookCalled = true;
try {
Expand All @@ -297,7 +326,9 @@ public void run() {

class DestroyEntryTestHook extends AbstractTestHook {

DestroyEntryTestHook(Region r) {
DestroyEntryTestHook(Region r, int testHookSpot) {

super(testHookSpot);
this.r = r;
}

Expand All @@ -312,7 +343,8 @@ public void doOp() {

class InvalidateEntryTestHook extends AbstractTestHook {

InvalidateEntryTestHook(Region r) {
InvalidateEntryTestHook(Region r, int testHookSpot) {
super(testHookSpot);
this.r = r;
}

Expand All @@ -327,7 +359,8 @@ public void doOp() {

class PutEntryTestHook extends AbstractTestHook {

PutEntryTestHook(Region r) {
PutEntryTestHook(Region r, int testHookSpot) {
super(testHookSpot);
this.r = r;
}

Expand Down

0 comments on commit c2b1c8b

Please sign in to comment.