You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/05/18 19:46:07 UTC

[GitHub] [incubator-pinot] jackjlli opened a new pull request #6936: Add SERVER_PROCESSING and NETTY_CONNECTION to broker query phase metrics

jackjlli opened a new pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936


   ## Description
   Currently even though there is a scatter & gather broker metric, it's hard to tell whether the time is mostly spent in Netty connection or spent in server query processing. 
   This PR adds these two more metrics into the broker query phase metrics to clearly denote two different values.
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   * [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   * [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR otherwise need attention when creating release notes? Things to consider:
   - New configuration options
   - Deprecation of configurations
   - Signature changes to public methods/interfaces
   - New plugins added or old plugins removed
   * [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
   ## Release Notes
   <!-- If you have tagged this as either backward-incompat or release-notes,
   you MUST add text here that you would like to see appear in release notes of the
   next release. -->
   
   <!-- If you have a series of commits adding or enabling a feature, then
   add this section only in final commit that marks the feature completed.
   Refer to earlier release notes to see examples of text.
   -->
   ## Documentation
   <!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
   See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
   -->
   


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

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

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635587341



##########
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:
       It excludes the query processing time. See code in Instance Request Handler




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636622401



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -69,13 +78,18 @@ public int getDeserializationTimeMs() {
   @Override
   public String toString() {
     return String
-        .format("%d,%d,%d,%d", getSubmitDelayMs(), getResponseDelayMs(), getResponseSize(), getDeserializationTimeMs());
+        .format("%d,%d,%d,%d,%d", getSubmitDelayMs(), getRequestSentDelayMs(), getResponseDelayMs(), getResponseSize(),

Review comment:
       better to add new metric at the end to keep any existing log parsing (that devs may  have for debugging) happy




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

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

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635484008



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java
##########
@@ -102,8 +103,17 @@ protected BrokerResponseNative processBrokerRequest(long requestId, BrokerReques
         dataTableMap.put(entry.getKey(), dataTable);
         totalResponseSize += serverResponse.getResponseSize();
       }
+      long serverProcessingTimeMs = serverResponse.getServerProcessingTimeMs();
+      if (serverProcessingTimeMs > -1) {

Review comment:
       Do we need this check?

##########
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:
       We have often seen the case when either server or broker goes through a gc just as the response is being sent by server or received by broker. In this case, gc time will show up as  the "network time" you are measuring? On the server side, it will be the gc time included as "server time", but on the broker, if it receives the bytes from server and then slips into gc (presumably to copy the bytes in, so it is likely to happen), then we will see the gc time as netty time, right?
   
   What exactly is the problem faced that we could not get an answer for using the current set of metrics?




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

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

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635481739



##########
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:
       The metric you mentioned doesn't exclude the time spent in processing the query in server, which isn't the purpose of this PR.




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

[GitHub] [incubator-pinot] siddharthteotia merged pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936


   


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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636519978



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -50,6 +51,14 @@ public int getSubmitDelayMs() {
     }
   }
 
+  public int getRequestSentDelayMs() {

Review comment:
       If we need this value to be logged in ServerStats like other stats are logged in BaseBrokerRequestHandler, then let's change the toString implementation of ServerResponse since that is what is called by `AsyncQueryResponse.getStats()`




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636622553



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java
##########
@@ -73,7 +73,7 @@ public AsyncQueryResponse(QueryRouter queryRouter, long requestId, Set<ServerRou
    */
   public String getStats() {
     StringBuilder stringBuilder =
-        new StringBuilder("(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs)");
+        new StringBuilder("(Server=SubmitDelayMs,RequestSentDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs)");

Review comment:
       Add at the end?




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636626870



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -50,6 +51,14 @@ public int getSubmitDelayMs() {
     }
   }
 
+  public int getRequestSentDelayMs() {
+    if (_sendRequestTimeMs != 0) {
+      return (int) (_sendRequestTimeMs - _startTimeMs);
+    } else {
+      return -1;
+    }

Review comment:
       @jackjlli , this seems incorrect. this is for time it took to send request on transport so although you are emitting in the metrics correctly , here since the delta is computed from start time, it gets recorded incorrectly




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

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

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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636519362



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -50,6 +51,14 @@ public int getSubmitDelayMs() {
     }
   }
 
+  public int getRequestSentDelayMs() {

Review comment:
       This is not being called anywhere so it won't be used and that's why I suggested in previous comment that you probably don't need to `asyncQueryResponse.markRequestSent(serverRoutingInstance);`




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636622401



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -69,13 +78,18 @@ public int getDeserializationTimeMs() {
   @Override
   public String toString() {
     return String
-        .format("%d,%d,%d,%d", getSubmitDelayMs(), getResponseDelayMs(), getResponseSize(), getDeserializationTimeMs());
+        .format("%d,%d,%d,%d,%d", getSubmitDelayMs(), getRequestSentDelayMs(), getResponseDelayMs(), getResponseSize(),

Review comment:
       better to add new metric at the end




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

[GitHub] [incubator-pinot] codecov-commenter commented on pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#issuecomment-845664511


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6936](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e17af0c) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/f4ae3e0a0a46f75bd284a852c936bcb27bab1aa1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f4ae3e0) will **decrease** coverage by `7.93%`.
   > The diff coverage is `61.03%`.
   
   > :exclamation: Current head e17af0c differs from pull request most recent head 9dc309a. Consider uploading reports for the commit 9dc309a to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6936/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6936      +/-   ##
   ============================================
   - Coverage     73.28%   65.35%   -7.94%     
     Complexity       12       12              
   ============================================
     Files          1432     1432              
     Lines         70970    70986      +16     
     Branches      10288    10288              
   ============================================
   - Hits          52013    46391    -5622     
   - Misses        15486    21249    +5763     
   + Partials       3471     3346     -125     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `?` | `?` | |
   | unittests | `65.35% <61.03%> (-0.02%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...rg/apache/pinot/core/transport/ServerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvU2VydmVyUmVzcG9uc2UuamF2YQ==) | `65.38% <28.57%> (-29.86%)` | `0.00 <0.00> (ø)` | |
   | [...ocal/recordtransformer/ComplexTypeTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9Db21wbGV4VHlwZVRyYW5zZm9ybWVyLmphdmE=) | `56.28% <47.61%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...e/pinot/core/transport/InstanceRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvSW5zdGFuY2VSZXF1ZXN0SGFuZGxlci5qYXZh) | `57.14% <50.00%> (-20.80%)` | `0.00 <0.00> (ø)` | |
   | [...ain/java/org/apache/pinot/spi/utils/JsonUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvSnNvblV0aWxzLmphdmE=) | `66.08% <50.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...pache/pinot/plugin/inputformat/avro/AvroUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvVXRpbHMuamF2YQ==) | `69.93% <60.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/common/metrics/BrokerTimer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJUaW1lci5qYXZh) | `92.30% <100.00%> (+0.64%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/common/metrics/ServerTimer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJUaW1lci5qYXZh) | `80.00% <100.00%> (-10.00%)` | `0.00 <0.00> (ø)` | |
   | [...pache/pinot/core/transport/AsyncQueryResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvQXN5bmNRdWVyeVJlc3BvbnNlLmphdmE=) | `78.94% <100.00%> (-15.50%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/core/transport/QueryRouter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvUXVlcnlSb3V0ZXIuamF2YQ==) | `88.15% <100.00%> (-2.64%)` | `0.00 <0.00> (ø)` | |
   | [...rg/apache/pinot/core/transport/ServerChannels.java](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvU2VydmVyQ2hhbm5lbHMuamF2YQ==) | `76.92% <100.00%> (+3.01%)` | `0.00 <0.00> (ø)` | |
   | ... and [346 more](https://codecov.io/gh/apache/incubator-pinot/pull/6936/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f4ae3e0...9dc309a](https://codecov.io/gh/apache/incubator-pinot/pull/6936?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636631845



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -50,6 +51,14 @@ public int getSubmitDelayMs() {
     }
   }
 
+  public int getRequestSentDelayMs() {
+    if (_sendRequestTimeMs != 0) {
+      return (int) (_sendRequestTimeMs - _startTimeMs);
+    } else {
+      return -1;
+    }

Review comment:
       The following is correct for existing `getSubmitDelayMs()` API
   
   ```
   public int getSubmitDelayMs() {                          
     if (_submitRequestTimeMs != 0) {                       
       return (int) (_submitRequestTimeMs - _startTimeMs);  
     } else {                                               
       return -1;                                           
     }                                                      
   }  
   ```
   
   But what you are measuring is delay it took on the wire `getRequestSentDelayMs()`, so you should no rely on _startTime. Simply change the API to record the time
   
   
   ```
   void markRequestSent(ServerRoutingInstance serverRoutingInstance, long timeMs) {
       _responseMap.get(serverRoutingInstance).markRequestSent(timeMs);
     }
   ```




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

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

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635478989



##########
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:
       The primary aim for this PR is to extract the time spent on netty from the time spent on scatter_gather. We should have a metric to actually measure the network time. Now that the broker query phase has all the phases from end to end, it'd be good to have the server total query time here instead of leaving it to the server metrics.




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

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635621036



##########
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:
       Refactored the code to put `NETTY_CONNECTION_SEND_REQUEST_LATENCY` to broker timer metrics. 




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636519034



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java
##########
@@ -144,7 +148,12 @@ synchronized void sendRequest(InstanceRequest instanceRequest)
             System.currentTimeMillis() - startTime);
       }
       byte[] requestBytes = _serializer.serialize(instanceRequest);
-      _channel.writeAndFlush(Unpooled.wrappedBuffer(requestBytes), _channel.voidPromise());
+      long sendRequestStartTimeMs = System.currentTimeMillis();
+      _channel.writeAndFlush(Unpooled.wrappedBuffer(requestBytes)).addListener(f -> {
+        _brokerMetrics.addTimedTableValue(rawTableName, BrokerTimer.NETTY_CONNECTION_SEND_REQUEST_LATENCY,
+            (System.currentTimeMillis() - sendRequestStartTimeMs), TimeUnit.MILLISECONDS);
+        asyncQueryResponse.markRequestSent(serverRoutingInstance);

Review comment:
       You probably don't need to do markRequestSent. 




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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636520327



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java
##########
@@ -29,7 +29,9 @@
   // metric tracking the freshness lag for consuming segments
   FRESHNESS_LAG_MS("freshnessLagMs", false),
 
-  NETTY_CONNECTION_SEND_RESPONSE_LATENCY("nettyConnection", true),
+  // The latency of sending the response from server to broker
+  NETTY_CONNECTION_SEND_RESPONSE_LATENCY("nettyConnection", false),

Review comment:
       I hope this is backward compatible change since name is not getting changed ?




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

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636520969



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java
##########
@@ -29,7 +29,9 @@
   // metric tracking the freshness lag for consuming segments
   FRESHNESS_LAG_MS("freshnessLagMs", false),
 
-  NETTY_CONNECTION_SEND_RESPONSE_LATENCY("nettyConnection", true),
+  // The latency of sending the response from server to broker
+  NETTY_CONNECTION_SEND_RESPONSE_LATENCY("nettyConnection", false),

Review comment:
       Yes, in fact the boolean isn't get used anywhere. So it's backward compatible.




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

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635619654



##########
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:
       We couldn't avoid gc from happening. Wherever the gc happens is the place we should account for the time. I've just refactored the code in the latest push, which makes it more clear. Basically the scatter & gather consists of 3 parts: 
   * request sent from broker to server
   * query being processed (totalQueryTime)
   * response sent from server to broker
   
   If gc happens on any of the steps above, it should be measured in just that step.




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

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

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635587341



##########
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:
       It excludes the query processing time. See code in Instance Request Handler. The callback gets fired once the socket I/O via netty completes




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

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r636523494



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerResponse.java
##########
@@ -50,6 +51,14 @@ public int getSubmitDelayMs() {
     }
   }
 
+  public int getRequestSentDelayMs() {

Review comment:
       I think it's still worthwhile to keep it in ServerResponse, which makes it clearer about the delay from broker to server in the log. I'll add it to the toString method in the next push.




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

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6936: Add NETTY_CONNECTION_SEND_REQUEST_LATENCY to broker timer metrics

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6936:
URL: https://github.com/apache/incubator-pinot/pull/6936#discussion_r635619758



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java
##########
@@ -102,8 +103,17 @@ protected BrokerResponseNative processBrokerRequest(long requestId, BrokerReques
         dataTableMap.put(entry.getKey(), dataTable);
         totalResponseSize += serverResponse.getResponseSize();
       }
+      long serverProcessingTimeMs = serverResponse.getServerProcessingTimeMs();
+      if (serverProcessingTimeMs > -1) {

Review comment:
       Code refactored.




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