[GitHub] [geode] agingade commented on a change in pull request #6874: GEODE-9602: QueryObserver improvements.

[GitBox <gi...@apache.org>]

agingade commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r711286483



##########
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.
   
   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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org