albertogpz commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r712233082
##########
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:
> It looks like this is not going to achieve what you are looking for:
>
> > The QueryObserverHolder class is not thread-safe.
>
> As Kirk pointed out, you have to make this as a ThreadLocal and set/reset
while in use.
> Other option could be making this as non-static...If the query context is
available in all the places where query-observer is getting used, you can think
of creating a new QueryObserver and setting it on the context.
> QueryContext.setObserver(new QueryObserver()); And no static
implementation/use of query observer.
>
That's actually what I tried to do with this proposal. Am I missing
something here?
> I did not see any test changes where concurrent queries are using
QueryObserver...It will show if the query observers are cross referenced...
--
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]
