kirklund commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r710580895
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java
##########
@@ -1826,9 +1828,14 @@ void addValuesToCollection(Collection result,
CompiledValue iterOp, RuntimeItera
// Compare the value in index with value in RegionEntry.
ok = verifyEntryAndIndexValue(entry, o1, context);
}
- if (ok && runtimeItr != null) {
- runtimeItr.setCurrent(o1);
- ok = QueryUtils.applyCondition(iterOp, context);
+ try {
+ if (ok && runtimeItr != null) {
+ runtimeItr.setCurrent(o1);
+ observer.beforeIterationEvaluation(iterOp, o1);
+ ok = QueryUtils.applyCondition(iterOp, context);
+ }
+ } finally {
+ observer.afterIterationEvaluation(ok);
Review comment:
I think you should try to extract the blocks that look like this to
their own private methods. There is some repetition involved that you may be
able to reduce down to one method. The code calling these methods will also
become more readable.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java
##########
@@ -54,26 +54,26 @@
/**
* Set the given observer to be notified of query events. Returns the
current observer.
*/
- public static QueryObserver setInstance(QueryObserver observer) {
+ public static synchronized QueryObserver setInstance(QueryObserver observer)
{
Review comment:
Rather than using `synchronized`, you'd be better off making `_instance`
a `private static final AtomicReference<QueryObserver>`. The use of
`synchronized` will cause at least some JVM implementations to pin every thread
hitting it to the same CPU.
Using atomics is generally always a better approach if you can use it.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryObserverHolder.java
##########
@@ -54,26 +54,26 @@
/**
* Set the given observer to be notified of query events. Returns the
current observer.
*/
- public static QueryObserver setInstance(QueryObserver observer) {
+ public static synchronized QueryObserver setInstance(QueryObserver observer)
{
Support.assertArg(observer != null, "setInstance expects a non-null
argument!");
QueryObserver oldObserver = _instance;
_instance = observer;
return oldObserver;
}
- public static boolean hasObserver() {
+ public static synchronized boolean hasObserver() {
return _instance != NO_OBSERVER;
}
/** Return the current QueryObserver instance */
- public static QueryObserver getInstance() {
+ public static synchronized QueryObserver getInstance() {
Review comment:
I've wanted to make this class thread-safe too. There is one problem
with doing so though. `getInstance()` is in the path of every query execution
and index evaluate call. If you make it `synchronized`, then there will be a
significant performance penalty inflicted on every query and index evaluation.
To see this, look at the call hierarchy of constructing a new
ExecutionConext. Every new instance will invoke:
```java
private QueryObserver observer = QueryObserverHolder.getInstance();
```
In order to make this class thread-safe, I think we need to solve the
problem of how to get queries and index evaluations from hitting those
`synchronized` blocks. I'm not as familiar with querying or indexing so you may
need to chat about it further with Anil or others.
Another problem is that the new `private QueryObserver observer` field in
`ExecutionContext` is itself not thread-safe, so the setter and getter you
added are not thread-safe either.
I think your intention is to have a test invoke `setObserver` for one or
more direct invocations of a query. If that's true, then you're probably better
off introducing a `ThreadLocal` to `ExecutionContext` for this purpose. Then
the execution would check that `ThreadLocal` for a non-null value and then use
it. If it's null, the execution would fallback to using the usual `observer`
stored in the field.
Then you could make the field itself `final` in the hopes that it would be
used multiple times. But again, I suspect that every query invocation creates a
new `ExecutionContext` which will perform poorly because of the `synchronized`
in `QueryObserverHolder`.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -803,8 +809,13 @@ private void addToResultsFromEntries(Object lowerBoundKey,
Object upperBoundKey,
if (runtimeItr != null) {
runtimeItr.setCurrent(value);
}
- if (ok && runtimeItr != null && iterOps != null) {
- ok = QueryUtils.applyCondition(iterOps, context);
+ try {
+ if (ok && runtimeItr != null) {
+ observer.beforeIterationEvaluation(iterOps, value);
+ ok = QueryUtils.applyCondition(iterOps, context);
+ }
+ } finally {
+ observer.afterIterationEvaluation(ok);
Review comment:
This class also has several try-finally blocks like that this may
benefit by being extracted as a new private method both for readability and to
hopefully reduce some of the repetition in these blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
