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/10/08 01:56:44 UTC

[GitHub] [pinot] Jackie-Jiang opened a new pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Jackie-Jiang opened a new pull request #7544:
URL: https://github.com/apache/pinot/pull/7544


   Currently even when thread cpu time measurement is disabled, the broker response has non-zero values for `OfflineThreadCpuTimeNs` and `RealtimeThreadCpuTimeNs` which can be misleading. We want to return 0 when the feature is disabled as a clear indication.


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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] Jackie-Jiang merged pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7544:
URL: https://github.com/apache/pinot/pull/7544


   


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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725186688



##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       Gotcha, didn't aware of that. 

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       Yes, your understanding is correct. Feel free to rephrase the comments if it's confusing.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */
+      long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+      int numServerThreads = intermediateResultsBlock.getNumServerThreads();
+      long totalThreadCpuTimeNs =
+          calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
+
+      instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
+          .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
+      return instanceResponseBlock;
+    } else {
+      return new InstanceResponseBlock(getCombinedResults());
+    }

Review comment:
       Agree




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] Jackie-Jiang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725174677



##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       That is not true. I explicitly put this check in case the testing environment does not support `currentThreadCpuTime`. Let me add some comments in the test




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] codecov-commenter edited a comment on pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#issuecomment-938287334


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7544?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 [#7544](https://codecov.io/gh/apache/pinot/pull/7544?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85735c5) into [master](https://codecov.io/gh/apache/pinot/commit/12b106bb27013c3d845930e2c7a5c629b662f778?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b106b) will **increase** coverage by `33.58%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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    #7544       +/-   ##
   =============================================
   + Coverage     32.27%   65.86%   +33.58%     
   - Complexity        0     3407     +3407     
   =============================================
     Files          1514     1477       -37     
     Lines         75334    73834     -1500     
     Branches      11002    10840      -162     
   =============================================
   + Hits          24317    48628    +24311     
   + Misses        48921    21752    -27169     
   - Partials       2096     3454     +1358     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `69.85% <100.00%> (?)` | |
   | unittests2 | `15.25% <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/pinot/pull/7544?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `85.24% <ø> (+1.63%)` | :arrow_up: |
   | [.../pinot/core/operator/InstanceResponseOperator.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9JbnN0YW5jZVJlc3BvbnNlT3BlcmF0b3IuamF2YQ==) | `83.87% <100.00%> (+1.11%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGhyZWFkVGltZXIuamF2YQ==) | `95.45% <100.00%> (+0.21%)` | :arrow_up: |
   | [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...he/pinot/common/messages/SegmentReloadMessage.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvU2VnbWVudFJlbG9hZE1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/core/data/manager/realtime/TimerService.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvVGltZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1227 more](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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/pinot/pull/7544?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 [12b106b...85735c5](https://codecov.io/gh/apache/pinot/pull/7544?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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r724762527



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */
+      long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+      int numServerThreads = intermediateResultsBlock.getNumServerThreads();
+      long totalThreadCpuTimeNs =
+          calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
+
+      instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
+          .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
+      return instanceResponseBlock;
+    } else {
+      return new InstanceResponseBlock(getCombinedResults());
+    }

Review comment:
       Can further simplify the code in this way:
   ```
   long startWallClockTimeNs = System.nanoTime();
   IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
   InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
   
   if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
       long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
       long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
       int numServerThreads = intermediateResultsBlock.getNumServerThreads();
       long totalThreadCpuTimeNs =
             calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
       instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
             .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
   }
   
   return instanceResponseBlock;
   
   ```




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] richardstartin commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725192663



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       To be honest I think wall time should include time spent recording CPU time, which would quickly reveal how expensive it is to record CPU time at fine granularity.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r724763930



##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       This if check is redundant, it must return true as you explicitly enable it in the previous line of code.

##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       This if check is redundant, it must return true as you just explicitly enable it in the previous line of code.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] richardstartin commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r724851383



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       This isn't really clear to me. Why should recording CPU time affect the wall time?




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#issuecomment-938409398


   Minor comments. LGTM otherwise


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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725184548



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */
+      long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+      int numServerThreads = intermediateResultsBlock.getNumServerThreads();
+      long totalThreadCpuTimeNs =
+          calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
+
+      instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
+          .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
+      return instanceResponseBlock;
+    } else {
+      return new InstanceResponseBlock(getCombinedResults());
+    }

Review comment:
       Agree.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       Yes. your understanding is right. Feel free to re-phrase the comments if it's confusing.

##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       Gotcha, didn't aware of that. 

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */
+      long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+      int numServerThreads = intermediateResultsBlock.getNumServerThreads();
+      long totalThreadCpuTimeNs =
+          calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
+
+      instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
+          .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
+      return instanceResponseBlock;
+    } else {
+      return new InstanceResponseBlock(getCombinedResults());
+    }

Review comment:
       OK. Agree.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] codecov-commenter edited a comment on pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#issuecomment-938287334


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7544?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 [#7544](https://codecov.io/gh/apache/pinot/pull/7544?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3ced382) into [master](https://codecov.io/gh/apache/pinot/commit/12b106bb27013c3d845930e2c7a5c629b662f778?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b106b) will **increase** coverage by `37.58%`.
   > The diff coverage is `100.00%`.
   
   > :exclamation: Current head 3ced382 differs from pull request most recent head e048352. Consider uploading reports for the commit e048352 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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    #7544       +/-   ##
   =============================================
   + Coverage     32.27%   69.86%   +37.58%     
   - Complexity        0     3327     +3327     
   =============================================
     Files          1514     1129      -385     
     Lines         75334    53480    -21854     
     Branches      11002     8061     -2941     
   =============================================
   + Hits          24317    37362    +13045     
   + Misses        48921    13475    -35446     
   - Partials       2096     2643      +547     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `69.86% <100.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/pinot/pull/7544?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../org/apache/pinot/core/common/MinionConstants.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vTWluaW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (ø)` | |
   | [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `85.24% <ø> (+1.63%)` | :arrow_up: |
   | [.../pinot/core/operator/InstanceResponseOperator.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9JbnN0YW5jZVJlc3BvbnNlT3BlcmF0b3IuamF2YQ==) | `83.87% <100.00%> (+1.11%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGhyZWFkVGltZXIuamF2YQ==) | `95.45% <100.00%> (+0.21%)` | :arrow_up: |
   | [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...he/pinot/common/messages/SegmentReloadMessage.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvU2VnbWVudFJlbG9hZE1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/core/data/manager/realtime/TimerService.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvVGltZXJTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1296 more](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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/pinot/pull/7544?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 [12b106b...e048352](https://codecov.io/gh/apache/pinot/pull/7544?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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] richardstartin commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725192131



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       Fair enough.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] codecov-commenter commented on pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

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


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7544?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 [#7544](https://codecov.io/gh/apache/pinot/pull/7544?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85735c5) into [master](https://codecov.io/gh/apache/pinot/commit/12b106bb27013c3d845930e2c7a5c629b662f778?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b106b) will **decrease** coverage by `17.01%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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    #7544       +/-   ##
   =============================================
   - Coverage     32.27%   15.25%   -17.02%     
   - Complexity        0       80       +80     
   =============================================
     Files          1514     1477       -37     
     Lines         75334    73834     -1500     
     Branches      11002    10840      -162     
   =============================================
   - Hits          24317    11267    -13050     
   - Misses        48921    61757    +12836     
   + Partials       2096      810     -1286     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests2 | `15.25% <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/pinot/pull/7544?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../pinot/core/operator/InstanceResponseOperator.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9JbnN0YW5jZVJlc3BvbnNlT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-82.76%)` | :arrow_down: |
   | [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `0.00% <ø> (-83.61%)` | :arrow_down: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGhyZWFkVGltZXIuamF2YQ==) | `0.00% <0.00%> (-95.24%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/core/plan/GlobalPlanImplV0.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0dsb2JhbFBsYW5JbXBsVjAuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../org/apache/pinot/core/plan/SelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../org/apache/pinot/core/plan/TransformPlanNode.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1RyYW5zZm9ybVBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/common/metrics/BrokerMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7544/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [781 more](https://codecov.io/gh/apache/pinot/pull/7544/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/pinot/pull/7544?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/pinot/pull/7544?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 [12b106b...85735c5](https://codecov.io/gh/apache/pinot/pull/7544?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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] Jackie-Jiang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725178932



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */

Review comment:
       Not introduced in this PR, but based on my understanding, this comment wants to indicate that currently we only count the CPU time for the physical plan execution (`CombineOperator.nextBlock()`). Once other query execution steps are included, we should also count them when calculating the wall clock time. @mqliang Is this correct?




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] Jackie-Jiang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r725176071



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -19,61 +19,64 @@
 package org.apache.pinot.core.operator;
 
 import java.util.List;
-import org.apache.pinot.common.utils.DataTable;
 import org.apache.pinot.common.utils.DataTable.MetadataKey;
-import org.apache.pinot.core.common.Operator;
 import org.apache.pinot.core.operator.blocks.InstanceResponseBlock;
 import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.combine.BaseCombineOperator;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
 import org.apache.pinot.segment.spi.FetchContext;
 import org.apache.pinot.segment.spi.IndexSegment;
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
   private static final String OPERATOR_NAME = "InstanceResponseOperator";
 
-  private final Operator _operator;
+  private final BaseCombineOperator _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
 
-  public InstanceResponseOperator(Operator combinedOperator, List<IndexSegment> indexSegments,
+  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
       List<FetchContext> fetchContexts) {
-    _operator = combinedOperator;
+    _combineOperator = combinedOperator;
     _indexSegments = indexSegments;
     _fetchContexts = fetchContexts;
     _fetchContextSize = fetchContexts.size();
   }
 
   @Override
   protected InstanceResponseBlock getNextBlock() {
-    long startWallClockTimeNs = System.nanoTime();
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {
+      long startWallClockTimeNs = System.nanoTime();
+      IntermediateResultsBlock intermediateResultsBlock = getCombinedResults();
+      InstanceResponseBlock instanceResponseBlock = new InstanceResponseBlock(intermediateResultsBlock);
+      long totalWallClockTimeNs = System.nanoTime() - startWallClockTimeNs;
 
-    IntermediateResultsBlock intermediateResultsBlock;
+      /*
+       * If/when the threadCpuTime based instrumentation is done for other parts of execution (planning, pruning etc),
+       * we will have to change the wallClockTime computation accordingly. Right now everything under
+       * InstanceResponseOperator is the one that is instrumented with threadCpuTime.
+       */
+      long multipleThreadCpuTimeNs = intermediateResultsBlock.getExecutionThreadCpuTimeNs();
+      int numServerThreads = intermediateResultsBlock.getNumServerThreads();
+      long totalThreadCpuTimeNs =
+          calTotalThreadCpuTimeNs(totalWallClockTimeNs, multipleThreadCpuTimeNs, numServerThreads);
+
+      instanceResponseBlock.getInstanceResponseDataTable().getMetadata()
+          .put(MetadataKey.THREAD_CPU_TIME_NS.getName(), String.valueOf(totalThreadCpuTimeNs));
+      return instanceResponseBlock;
+    } else {
+      return new InstanceResponseBlock(getCombinedResults());
+    }

Review comment:
       I feel the current way is slightly more readable, and can save an extra `System.nanoTime()`. The enable check can be easily optimized away by the JIT, and should not have any performance impact.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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] [pinot] mqliang commented on a change in pull request #7544: Return 0 in the response metadata when thread cpu time measurement is disabled

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #7544:
URL: https://github.com/apache/pinot/pull/7544#discussion_r724763930



##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentResultTableSingleValueQueriesTest.java
##########
@@ -1099,4 +1100,21 @@ public void testSelection() {
       Assert.assertEquals(resultTable.getRows().get(i), rows.get(i));
     }
   }
+
+  @Test
+  public void testThreadCpuTime() {
+    String query = "SELECT * FROM testTable";
+
+    ThreadTimer.setThreadCpuTimeMeasurementEnabled(true);
+    if (ThreadTimer.isThreadCpuTimeMeasurementEnabled()) {

Review comment:
       This if check is redundant, it must return true as you explicitly enable it in the previous line.




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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