[GitHub] [phoenix] joshelser commented on a diff in pull request #1431: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in Regi…

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

joshelser commented on code in PR #1431:
URL: https://github.com/apache/phoenix/pull/1431#discussion_r861966333


##########
phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java:
##########
@@ -38,4 +46,22 @@ public static MetricsStopWatch getMetricsStopWatch(boolean isRequestMetricsEnabl
         return new MetricsStopWatch(true);
     }
 
+    // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+    // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+    // See PHOENIX-6699
+    public static boolean isDefaultMetricsInitialized() {
+        try {
+            MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+            Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");

Review Comment:
   `prefix` isn't `volatile` and is set from within the `synchronized` `init(String)` method. This would mean that we may be initialized and not yet be able to observe it from this separate thread.
   
   That said, I don't know how likely this race condition would be to hit.
   
   We could also check the `monitoring` boolean instead of `prefix`. This would mean that we already have atomic access to that variable. I am assuming that the synchronization on the methods in MetricsSystemImpl would not affect this reflection, meaning we would still have a (small) window where `stop()` is being called but not yet complete.
   
   Maybe you have already thought about this :)



-- 
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: issues-unsubscribe@phoenix.apache.org

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