albertogpz commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r712232163
##########
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.
>
I would expect this to be a problem if the above calls are done many times
per query. But in this PR, it is proposed to invoke it just once, to get the
value and then set it in the `ExecutionContext`, right before the query is
executed. Later, when the query needs the value of the observer, it would get
it from the `ExecutionContext` using a non-synchronized call given that all the
calls to get the observer have been changed from
`QueryObserverHolder.getInstance()` to `context.getObserver()`.
> 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 am assuming that the `ExecutionContext` is not shared between threads and
therefore it does not need to be thread-safe. I am wrong here?
> 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.
>
Actually, my intention was just to make this class thread-safe but not
extend it to have new usages. Nevertheless, the class is documented and used so
that the observer can be set from any thread and be available to any thread in
the VM.
I have proposed the current solution but have also considered the use of a
`ThreadLocal` variable, not in the `ExecutionContext` class (which did not have
an `observer` field) but in the `QueryObserverHolder` class to be used in a way
similar to what you are proposing.
But I think the lifecycle of the `ThreadLocal` variable would be complex. It
should be reset after the query execution. But who would do it?
> 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]
