mhansonp commented on a change in pull request #6874:
URL: https://github.com/apache/geode/pull/6874#discussion_r712413479
##########
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 to me like you have that covered Alberto. By passing around
context, it looks to me like you have addressed many if not all the thread
safety issues. I tend to agree with Kirk about preferring and atomic to a
synchronize, though.
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/query/internal/QueryTraceJUnitTest.java
##########
@@ -366,7 +396,11 @@ public void
testTraceOnLocalRegionWithSmallTracePrefixNoComments() throws Except
assertTrue(((DefaultQuery) query).isTraced());
SelectResults results = (SelectResults) query.execute();
- assertTrue(QueryObserverHolder.getInstance() instanceof
IndexTrackingQueryObserver);
+
Review comment:
These tests seem awfully repetitive. Is there a reason to check the test
hook in every test? From my perspective it seems like a waste of CPU. Just my
opinion though.
--
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]
