diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryObserverCallbacksTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryObserverCallbacksTest.java index c4fe9946818e..98932af387c1 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryObserverCallbacksTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryObserverCallbacksTest.java @@ -18,6 +18,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -251,8 +252,8 @@ public void testBeforeAndAfterIterationEvaluateNoWhere() throws Exception { "select count(*) from " + SEPARATOR + "portfolio p"); query.execute(); - verify(myQueryObserver, times(0)).beforeIterationEvaluation(any(), any()); - verify(myQueryObserver, times(0)).afterIterationEvaluation(any()); + verify(myQueryObserver, never()).beforeIterationEvaluation(any(), any()); + verify(myQueryObserver, never()).afterIterationEvaluation(any()); } @Test diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryTraceJUnitTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryTraceJUnitTest.java index 07b97e258ebd..07e0e46fe9be 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryTraceJUnitTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryTraceJUnitTest.java @@ -324,6 +324,7 @@ public void testTraceOnLocalRegionWithTracePrefixNoComments() throws Exception { BeforeQueryExecutionHook hook = (BeforeQueryExecutionHook) DefaultQuery.testHook; assertThat(hook.getObserver()).isInstanceOf(IndexTrackingQueryObserver.class); + // The query should return all elements in region. assertEquals(region.size(), results.size()); QueryObserverHolder.reset(); } diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java index bae6b720fb0a..26a8a249cd73 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java +++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java @@ -14,6 +14,8 @@ */ package org.apache.geode.cache.query.internal; +import java.util.concurrent.atomic.AtomicReference; + import org.apache.geode.annotations.Immutable; import org.apache.geode.annotations.internal.MakeNotStatic; @@ -49,31 +51,30 @@ public class QueryObserverHolder { * The current observer which will be notified of all query events. */ @MakeNotStatic - private static QueryObserver _instance = NO_OBSERVER; + private static final AtomicReference _instance = + new AtomicReference<>(NO_OBSERVER); /** * Set the given observer to be notified of query events. Returns the current observer. */ - public static synchronized QueryObserver setInstance(QueryObserver observer) { + public static QueryObserver setInstance(QueryObserver observer) { Support.assertArg(observer != null, "setInstance expects a non-null argument!"); - QueryObserver oldObserver = _instance; - _instance = observer; - return oldObserver; + return _instance.getAndSet(observer); } - public static synchronized boolean hasObserver() { - return _instance != NO_OBSERVER; + public static boolean hasObserver() { + return _instance.get() != NO_OBSERVER; } /** Return the current QueryObserver instance */ - public static synchronized QueryObserver getInstance() { - return _instance; + public static QueryObserver getInstance() { + return _instance.get(); } /** * Only for test purposes. */ - public static synchronized void reset() { - _instance = NO_OBSERVER; + public static void reset() { + _instance.set(NO_OBSERVER); } } diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java index 85706867ccea..aef2d0332923 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java +++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java @@ -1824,19 +1824,8 @@ void addValuesToCollection(Collection result, CompiledValue iterOp, RuntimeItera synchronized (value) { for (Object o1 : ((Iterable) value)) { boolean ok = true; - if (reUpdateInProgress) { - // Compare the value in index with value in RegionEntry. - ok = verifyEntryAndIndexValue(entry, o1, context); - } - try { - if (ok && runtimeItr != null) { - runtimeItr.setCurrent(o1); - observer.beforeIterationEvaluation(iterOp, o1); - ok = QueryUtils.applyCondition(iterOp, context); - } - } finally { - observer.afterIterationEvaluation(ok); - } + ok = applyCondition(iterOp, runtimeItr, context, observer, o1, entry, + reUpdateInProgress, ok); if (ok) { applyProjection(projAttrib, context, result, o1, intermediateResults, isIntersection); @@ -1849,19 +1838,8 @@ void addValuesToCollection(Collection result, CompiledValue iterOp, RuntimeItera } else { for (Object o1 : ((Iterable) value)) { boolean ok = true; - if (reUpdateInProgress) { - // Compare the value in index with value in RegionEntry. - ok = verifyEntryAndIndexValue(entry, o1, context); - } - try { - if (ok && runtimeItr != null) { - runtimeItr.setCurrent(o1); - observer.beforeIterationEvaluation(iterOp, o1); - ok = QueryUtils.applyCondition(iterOp, context); - } - } finally { - observer.afterIterationEvaluation(ok); - } + ok = applyCondition(iterOp, runtimeItr, context, observer, o1, entry, + reUpdateInProgress, ok); if (ok) { applyProjection(projAttrib, context, result, o1, intermediateResults, isIntersection); @@ -1873,19 +1851,8 @@ void addValuesToCollection(Collection result, CompiledValue iterOp, RuntimeItera } } else { boolean ok = true; - if (reUpdateInProgress) { - // Compare the value in index with in RegionEntry. - ok = verifyEntryAndIndexValue(entry, value, context); - } - try { - if (ok && runtimeItr != null) { - runtimeItr.setCurrent(value); - observer.beforeIterationEvaluation(iterOp, value); - ok = QueryUtils.applyCondition(iterOp, context); - } - } finally { - observer.afterIterationEvaluation(ok); - } + ok = applyCondition(iterOp, runtimeItr, context, observer, value, entry, + reUpdateInProgress, ok); if (ok) { if (context.isCqQueryContext()) { result.add(new CqEntry(((RegionEntry) e.getKey()).getKey(), value)); @@ -1899,6 +1866,26 @@ void addValuesToCollection(Collection result, CompiledValue iterOp, RuntimeItera } } + private boolean applyCondition(CompiledValue iterOp, RuntimeIterator runtimeItr, + ExecutionContext context, QueryObserver observer, Object value, RegionEntry entry, + boolean reUpdateInProgress, boolean ok) throws FunctionDomainException, + TypeMismatchException, NameResolutionException, QueryInvocationTargetException { + if (reUpdateInProgress) { + // Compare the value in index with in RegionEntry. + ok = verifyEntryAndIndexValue(entry, value, context); + } + try { + if (ok && runtimeItr != null) { + runtimeItr.setCurrent(value); + observer.beforeIterationEvaluation(iterOp, value); + ok = QueryUtils.applyCondition(iterOp, context); + } + } finally { + observer.afterIterationEvaluation(ok); + } + return ok; + } + private boolean verifyLimit(Collection result, int limit, ExecutionContext context) { if (limit > 0) { if (!context.isDistinct()) { 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 cc9049addf9c..91d9b32e2d5c 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 @@ -805,18 +805,7 @@ private void addToResultsFromEntries(Object lowerBoundKey, Object upperBoundKey, value = iterator.next(); if (value != null) { boolean ok = true; - - if (runtimeItr != null) { - runtimeItr.setCurrent(value); - } - try { - if (ok && runtimeItr != null) { - observer.beforeIterationEvaluation(iterOps, value); - ok = QueryUtils.applyCondition(iterOps, context); - } - } finally { - observer.afterIterationEvaluation(ok); - } + ok = applyCondition(iterOps, runtimeItr, context, observer, value, ok); if (ok) { applyCqOrProjection(projAttrib, context, result, value, intermediateResults, isIntersection, indexEntry.getDeserializedRegionKey()); @@ -845,17 +834,7 @@ private void addToResultsFromEntries(Object lowerBoundKey, Object upperBoundKey, ok = evaluateEntry((IndexInfo) indexInfo, context, null); } - if (runtimeItr != null) { - runtimeItr.setCurrent(value); - } - try { - if (ok && runtimeItr != null) { - observer.beforeIterationEvaluation(iterOps, value); - ok = QueryUtils.applyCondition(iterOps, context); - } - } finally { - observer.afterIterationEvaluation(ok); - } + ok = applyCondition(iterOps, runtimeItr, context, observer, value, ok); if (ok) { if (context != null && context.isCqQueryContext()) { result.add(new CqEntry(indexEntry.getDeserializedRegionKey(), value)); @@ -879,6 +858,24 @@ private void addToResultsFromEntries(Object lowerBoundKey, Object upperBoundKey, } } + private boolean applyCondition(CompiledValue iterOps, RuntimeIterator runtimeItr, + ExecutionContext context, QueryObserver observer, Object value, boolean ok) + throws FunctionDomainException, TypeMismatchException, NameResolutionException, + QueryInvocationTargetException { + if (runtimeItr != null) { + runtimeItr.setCurrent(value); + } + try { + if (ok && runtimeItr != null) { + observer.beforeIterationEvaluation(iterOps, value); + ok = QueryUtils.applyCondition(iterOps, context); + } + } finally { + observer.afterIterationEvaluation(ok); + } + return ok; + } + public List expandValue(ExecutionContext context, Object lowerBoundKey, Object upperBoundKey, int lowerBoundOperator, int upperBoundOperator, Object value) { try {