[GitHub] [pulsar] jerrypeng commented on a change in pull request #7010: Fix null pointer when getting function instance metrics.

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

jerrypeng commented on a change in pull request #7010:
URL: https://github.com/apache/pulsar/pull/7010#discussion_r428924499



##########
File path: pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java
##########
@@ -546,13 +547,17 @@ synchronized public void close() {
         }
     }
 
-    public InstanceCommunication.MetricsData getAndResetMetrics() {
+    // This method is synchronized because it is using the stats variable
+    synchronized public InstanceCommunication.MetricsData getAndResetMetrics() {
         InstanceCommunication.MetricsData metricsData = getMetrics();
-        stats.reset();
+        if (stats != null) {
+            stats.reset();
+        }
         return metricsData;
     }
 
-    public InstanceCommunication.MetricsData getMetrics() {
+    // This method is synchronized because it is using the stats and javaInstance variables
+    synchronized public InstanceCommunication.MetricsData getMetrics() {

Review comment:
       Even though synchronized methods are re-entrant in Java, it still appears a little weird that a synchronized getMetrics() be called inside another synchronized method "getAndResetMetrics()".  Maybe have another "base" private method that gets called by both that is not synchronized.  A side note, I think in the future, we might want to define more interfaces for "modules" in functions, so that it is more clear on what methods should be exposed.




----------------------------------------------------------------
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.

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