[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add SERVER_PROCESSING and NETTY_CONNECTION to broker query phase metrics

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

siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635472281



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -81,5 +86,7 @@ void receiveDataTable(DataTable dataTable, int responseSize, int deserialization
     _dataTable = dataTable;
     _responseSize = responseSize;
     _deserializationTimeMs = deserializationTimeMs;
+    _serverProcessingTimeMs =

Review comment:
       So the way this is set on the server, this is ServerQueryPhase.QUERY_PROCESSING_TIME which is not the TOTAL_QUERY_TIME so won't include the conversion of DataTable.toBytes(). To measure the true wall clock time on the server, we need totalQueryTime I believe

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableImplV3.java
##########
@@ -86,6 +86,7 @@ public DataTableImplV3(int numRows, DataSchema dataSchema, Map<String, Map<Integ
    * Construct empty data table. (Server side)
    */
   public DataTableImplV3() {
+    super();

Review comment:
       Why this change?

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerQueryPhase.java
##########
@@ -30,6 +30,8 @@
   QUERY_EXECUTION,
   QUERY_ROUTING,
   SCATTER_GATHER,
+  SERVER_PROCESSING,
+  NETTY_CONNECTION,

Review comment:
       This I believe is used to measure the time it took for the server response to travel over the wire, arrive at broker and get deserialized. We already emit this from the server but I don't think it is at the table level
   See the following code in InstanceRequestHandler
   
   `_serverMetrics.addTimedValue(ServerTimer.NETTY_CONNECTION_SEND_RESPONSE_LATENCY, sendResponseLatencyMs, TimeUnit.MILLISECONDS);`

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerQueryPhase.java
##########
@@ -30,6 +30,8 @@
   QUERY_EXECUTION,
   QUERY_ROUTING,
   SCATTER_GATHER,
+  SERVER_PROCESSING,

Review comment:
       Why do we need to emit a server metric from broker ? Each server is already emitting its own totalQueryTime so I don't think we need this. Will lead to more confusion IMO. 
   
   




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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org