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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
