PHOENIX-4854 Make LoggingPhoenixResultSet idempotent when logging met…

…rics
apache · Aug 20, 2018 · 76e6f9d · 76e6f9d
1 parent f0b861b
commit 76e6f9d
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 26 deletions.
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java b/phoenix-core/src/it/java/org/apache/phoenix/monitoring/PhoenixLoggingMetricsIT.java
@@ -26,6 +26,7 @@
 import java.sql.Connection;
 import java.sql.DriverManager;
 import java.sql.ResultSet;
+import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.Map;
 
@@ -44,6 +45,8 @@ public class PhoenixLoggingMetricsIT extends BasePhoenixMetricsIT {
     private String tableName2;
     private LoggingPhoenixConnection loggedConn;
     private String loggedSql;
+    private int logOverAllReadRequestMetricsFuncCallCount;
+    private int logRequestReadMetricsFuncCallCount;
 
     @Before
     public void beforeTest() throws Exception {
@@ -69,17 +72,7 @@ public void beforeTest() throws Exception {
     public void testPhoenixMetricsLoggedOnCommit() throws Exception {
         // run SELECT to verify read metrics are logged
         String query = "SELECT * FROM " + tableName1;
-        Statement stmt = loggedConn.createStatement();
-        ResultSet rs = stmt.executeQuery(query);
-        while (rs.next()) {
-        }
-        rs.close();
-        assertTrue("Read metrics for not found for " + tableName1,
-                requestReadMetricsMap.get(tableName1).size() > 0);
-        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
-
-        assertTrue("Overall read metrics for not found ", overAllQueryMetricsMap.size() > 0);
-        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
+        verifyQueryLevelMetricsLogging(query);
 
         // run UPSERT SELECT to verify mutation metrics are logged
         String upsertSelect = "UPSERT INTO " + tableName2 + " SELECT * FROM " + tableName1;
@@ -117,17 +110,7 @@ public void testPhoenixMetricsLoggedOnCommit() throws Exception {
     public void testPhoenixMetricsLoggedOnClose() throws Exception {
         // run SELECT to verify read metrics are logged
         String query = "SELECT * FROM " + tableName1;
-        Statement stmt = loggedConn.createStatement();
-        ResultSet rs = stmt.executeQuery(query);
-        while (rs.next()) {
-        }
-        rs.close();
-        assertTrue("Read metrics for not found for " + tableName1,
-                requestReadMetricsMap.get(tableName1).size() > 0);
-        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
-
-        assertTrue("Overall read metrics for not found ", overAllQueryMetricsMap.size() > 0);
-        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
+        verifyQueryLevelMetricsLogging(query);
 
         // run UPSERT SELECT to verify mutation metrics are logged
         String upsertSelect = "UPSERT INTO " + tableName2 + " SELECT * FROM " + tableName1;
@@ -151,6 +134,26 @@ public void testPhoenixMetricsLoggedOnClose() throws Exception {
                 mutationReadMetricsMap.size() == 0);
     }
 
+    private void verifyQueryLevelMetricsLogging(String query) throws SQLException {
+        Statement stmt = loggedConn.createStatement();
+        ResultSet rs = stmt.executeQuery(query);
+        while (rs.next()) {
+        }
+        rs.close();
+        assertTrue("Read metrics for not found for " + tableName1,
+                requestReadMetricsMap.get(tableName1).size() > 0);
+        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
+        assertTrue(logOverAllReadRequestMetricsFuncCallCount == 1);
+        assertTrue(logRequestReadMetricsFuncCallCount == 1);
+
+        assertTrue("Overall read metrics for not found ", overAllQueryMetricsMap.size() > 0);
+        assertTrue("Logged query doesn't match actual query", loggedSql.equals(query));
+
+        rs.close();
+        assertTrue(logOverAllReadRequestMetricsFuncCallCount == 1);
+        assertTrue(logRequestReadMetricsFuncCallCount == 1);
+    }
+
     void clearAllTestMetricMaps() {
         overAllQueryMetricsMap.clear();
         requestReadMetricsMap.clear();
@@ -165,13 +168,15 @@ public void logOverAllReadRequestMetrics(
                     Map<MetricType, Long> overAllQueryMetrics, String sql) {
                 overAllQueryMetricsMap.putAll(overAllQueryMetrics);
                 loggedSql = sql;
+                logOverAllReadRequestMetricsFuncCallCount++;
             }
 
             @Override
             public void logRequestReadMetrics(
                     Map<String, Map<MetricType, Long>> requestReadMetrics, String sql) {
                 requestReadMetricsMap.putAll(requestReadMetrics);
                 loggedSql = sql;
+                logRequestReadMetricsFuncCallCount++;
             }
 
             @Override

diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/LoggingPhoenixResultSet.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/LoggingPhoenixResultSet.java
@@ -26,19 +26,26 @@ public class LoggingPhoenixResultSet extends DelegateResultSet {
 
     private PhoenixMetricsLog phoenixMetricsLog;
     private String sql;
+    private boolean areMetricsLogged;
 
     public LoggingPhoenixResultSet(ResultSet rs, PhoenixMetricsLog phoenixMetricsLog, String sql) {
         super(rs);
         this.phoenixMetricsLog = phoenixMetricsLog;
         this.sql = sql;
+        this.areMetricsLogged = false;
     }
 
     @Override
     public void close() throws SQLException {
-        phoenixMetricsLog.logOverAllReadRequestMetrics(PhoenixRuntime.getOverAllReadRequestMetricInfo(rs), sql);
-        phoenixMetricsLog.logRequestReadMetrics(PhoenixRuntime.getRequestReadMetricInfo(rs), sql);
-        PhoenixRuntime.resetMetrics(rs);
-        super.close();
+        if (!rs.isClosed()) {
+            super.close();
+        }
+        if (!this.areMetricsLogged) {
+            phoenixMetricsLog.logOverAllReadRequestMetrics(PhoenixRuntime.getOverAllReadRequestMetricInfo(rs), sql);
+            phoenixMetricsLog.logRequestReadMetrics(PhoenixRuntime.getRequestReadMetricInfo(rs), sql);
+            PhoenixRuntime.resetMetrics(rs);
+            this.areMetricsLogged = true;
+        }
     }
 
 }