kirklund commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r710577674
##########
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 on every invocation. 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`.
--
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]
