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/07 19:10:24 UTC

[GitHub] [pinot] Jackie-Jiang opened a new pull request #7540: Always enable the ThreadTimer

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


   Currently the thread CPU time measurement is disabled by default, and we return the wall-clock time in the response which can be quite misleading.
   
   In this PR:
   - Always enable the ThreadTimer
   - Use system nano time if current thread CPU time is unsupported
   - Simplify the usage of ThreadTimer


-- 
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 pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938904560


   Close this one because it might involve big overhead


-- 
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 #7540: Always enable the ThreadTimer

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



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -390,10 +390,6 @@ private CommonConstants() {
     public static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
         "org.apache.pinot.server.api.access.AllowAllAccessFactory";
 
-    public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =

Review comment:
       Same here, I am OK with: `DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = true;`. But don't delete it -- Let's allow user to opt-out ThreadTimer.

##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
##########
@@ -164,11 +163,6 @@ public void init(PinotConfiguration serverConf)
     // Initialize Pinot Environment Provider
     _pinotEnvironmentProvider = initializePinotEnvironmentProvider();
 
-    // Enable/disable thread CPU time measurement through instance config.

Review comment:
       Please don't delete this. I am OK with enabling ThreadTimer by default, but at least let's allow user to disable ThreadTimer.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/ThreadTimer.java
##########
@@ -28,42 +28,34 @@
  * The {@code ThreadTimer} class providing the functionality of measuring the CPU time for the current thread.
  */
 public class ThreadTimer {
+  private static final Logger LOGGER = LoggerFactory.getLogger(ThreadTimer.class);
   private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
   private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
-  private static final Logger LOGGER = LoggerFactory.getLogger(ThreadTimer.class);
-  private static boolean _isThreadCpuTimeMeasurementEnabled = false;
-  private long _startTimeNs = -1;
-  private long _endTimeNs = -1;
 
-  public ThreadTimer() {
-  }
+  private final long _startTimeNs;
 
-  public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
-    _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
-  }
-
-  public void start() {
-    if (_isThreadCpuTimeMeasurementEnabled) {
-      _startTimeNs = MX_BEAN.getCurrentThreadCpuTime();
-    }
-  }
-
-  public void stop() {
-    if (_isThreadCpuTimeMeasurementEnabled) {
-      _endTimeNs = MX_BEAN.getCurrentThreadCpuTime();
-    }
+  static {
+    LOGGER.info("Current thread cpu time measurement supported: {}", IS_CURRENT_THREAD_CPU_TIME_SUPPORTED);
   }
 
-  public long getThreadTimeNs() {
-    return _endTimeNs - _startTimeNs;
+  /**
+   * Constructs and starts the thread timer.
+   */
+  public ThreadTimer() {
+    _startTimeNs = getCurrentThreadCpuTime();
   }
 
+  /**
+   * Stops the thread timer and returns the thread CPU time in nanos.
+   */
   public long stopAndGetThreadTimeNs() {

Review comment:
       The function name `stopAndGetThreadTimeNs()` does not match the new logic, maybe change it as `getThreadTimeNsSinceStart()` or something similiar?




-- 
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 edited a comment on pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
mqliang edited a comment on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938413650


   @Jackie-Jiang 
   > Based on the discussion, I feel it is not a good idea to enable it by default. I'd suggest putting -1 as ThreadCpuTimeNs if it is not enabled as a clear indication to the users that it is not enabled as @richardstartin suggested. What do you think?
   
   I am not very concerned about the default value. I am more concerned about deleting the flag -- it's NOT a back compatible change, if user already start use the flag, after upgrading, pinot process will panic as user pass in a nonexistent flag.  
   
   Agree that disable by default as recording thread CPU time can be expensive.
   
   Agree that putting -1 or 0  as ThreadCpuTimeNs if it is not enabled.


-- 
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 pull request #7540: Always enable the ThreadTimer

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


   Recording thread CPU time can be expensive, and I think it should be possible to disable it and it should be disabled by default. When it is disabled, it would be better to return 0 or -1 rather than the wall time, because that _is_ confusing.


-- 
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 pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938190013


   Currently we return different values in the response metadata `ThreadCpuTimeNs` with the CPU thread time on/off, which is causing the confusion. Based on the discussion, I feel it is not a good idea to enable it by default. I'd suggest putting `-1` as `ThreadCpuTimeNs` if it is not enabled as a clear indication to the users that it is not enabled as @richardstartin suggested. What do you think? @siddharthteotia @mqliang 


-- 
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 closed pull request #7540: Always enable the ThreadTimer

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


   


-- 
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] siddharthteotia commented on pull request #7540: Always enable the ThreadTimer

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


   I don't think we should enable this by default


-- 
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 #7540: Always enable the ThreadTimer

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


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7540?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 [#7540](https://codecov.io/gh/apache/pinot/pull/7540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db05a8f) into [master](https://codecov.io/gh/apache/pinot/commit/c8aa3e39ccec1971dd71863c10b07254fe2cbc78?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8aa3e3) will **increase** coverage by `50.62%`.
   > The diff coverage is `83.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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    #7540       +/-   ##
   =============================================
   + Coverage     15.25%   65.87%   +50.62%     
   - Complexity       80     3414     +3334     
   =============================================
     Files          1477     1477               
     Lines         73824    73818        -6     
     Branches      10838    10840        +2     
   =============================================
   + Hits          11264    48631    +37367     
   + Misses        61753    21733    -40020     
   - Partials        807     3454     +2647     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests1 | `69.89% <83.33%> (?)` | |
   | unittests2 | `15.25% <0.00%> (-0.01%)` | :arrow_down: |
   
   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/7540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/core/common/datatable/DataTableImplV3.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZUltcGxWMy5qYXZh) | `96.00% <ø> (+96.00%)` | :arrow_up: |
   | [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `78.08% <ø> (+78.08%)` | :arrow_up: |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `26.31% <ø> (+26.31%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7540/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==) | `90.00% <83.33%> (+90.00%)` | :arrow_up: |
   | [...lix/core/realtime/PinotRealtimeSegmentManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90UmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `72.53% <0.00%> (-7.78%)` | :arrow_down: |
   | [...controller/helix/core/minion/PinotTaskManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdFRhc2tNYW5hZ2VyLmphdmE=) | `42.14% <0.00%> (-1.92%)` | :arrow_down: |
   | [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `2.71% <0.00%> (+1.16%)` | :arrow_up: |
   | [.../segment/spi/index/reader/MutableForwardIndex.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3JlYWRlci9NdXRhYmxlRm9yd2FyZEluZGV4LmphdmE=) | `4.76% <0.00%> (+4.76%)` | :arrow_up: |
   | [...he/pinot/segment/local/utils/SegmentPushUtils.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9TZWdtZW50UHVzaFV0aWxzLmphdmE=) | `5.26% <0.00%> (+5.26%)` | :arrow_up: |
   | [...ava/org/apache/pinot/core/transport/TlsConfig.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvVGxzQ29uZmlnLmphdmE=) | `5.88% <0.00%> (+5.88%)` | :arrow_up: |
   | ... and [1003 more](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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/7540?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 [c8aa3e3...db05a8f](https://codecov.io/gh/apache/pinot/pull/7540?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 pull request #7540: Always enable the ThreadTimer

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


   Recording thread CPU time can be expensive, and I think it should be possible to disable it and it should be disabled by default. When it is disabled, it would be better to return 0 or -1 rather than the wall time, because that _is_ confusing.


-- 
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] siddharthteotia commented on a change in pull request #7540: Always enable the ThreadTimer

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



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -390,10 +390,6 @@ private CommonConstants() {
     public static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
         "org.apache.pinot.server.api.access.AllowAllAccessFactory";
 
-    public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =

Review comment:
       +1 on not deleting the config




-- 
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] siddharthteotia commented on pull request #7540: Always enable the ThreadTimer

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


   We measure, log and emit threadCpuTime separately from wall clock time. Wall clock time is also logged and emitted separately. Having threadCpuTime as 0 or -1 when JVM does not support it should avoid any confusion with wall clock 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] codecov-commenter edited a comment on pull request #7540: Always enable the ThreadTimer

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






-- 
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] siddharthteotia commented on pull request #7540: Always enable the ThreadTimer

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






-- 
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 #7540: Always enable the ThreadTimer

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


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7540?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 [#7540](https://codecov.io/gh/apache/pinot/pull/7540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db05a8f) into [master](https://codecov.io/gh/apache/pinot/commit/c8aa3e39ccec1971dd71863c10b07254fe2cbc78?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8aa3e3) will **increase** coverage by `50.62%`.
   > The diff coverage is `83.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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    #7540       +/-   ##
   =============================================
   + Coverage     15.25%   65.87%   +50.62%     
   - Complexity       80     3414     +3334     
   =============================================
     Files          1477     1477               
     Lines         73824    73818        -6     
     Branches      10838    10840        +2     
   =============================================
   + Hits          11264    48631    +37367     
   + Misses        61753    21733    -40020     
   - Partials        807     3454     +2647     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests1 | `69.89% <83.33%> (?)` | |
   | unittests2 | `15.25% <0.00%> (-0.01%)` | :arrow_down: |
   
   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/7540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/core/common/datatable/DataTableImplV3.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZUltcGxWMy5qYXZh) | `96.00% <ø> (+96.00%)` | :arrow_up: |
   | [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `78.08% <ø> (+78.08%)` | :arrow_up: |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `26.31% <ø> (+26.31%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7540/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==) | `90.00% <83.33%> (+90.00%)` | :arrow_up: |
   | [...lix/core/realtime/PinotRealtimeSegmentManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90UmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `72.53% <0.00%> (-7.78%)` | :arrow_down: |
   | [...controller/helix/core/minion/PinotTaskManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdFRhc2tNYW5hZ2VyLmphdmE=) | `42.14% <0.00%> (-1.92%)` | :arrow_down: |
   | [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `2.71% <0.00%> (+1.16%)` | :arrow_up: |
   | [.../segment/spi/index/reader/MutableForwardIndex.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3JlYWRlci9NdXRhYmxlRm9yd2FyZEluZGV4LmphdmE=) | `4.76% <0.00%> (+4.76%)` | :arrow_up: |
   | [...he/pinot/segment/local/utils/SegmentPushUtils.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9TZWdtZW50UHVzaFV0aWxzLmphdmE=) | `5.26% <0.00%> (+5.26%)` | :arrow_up: |
   | [...ava/org/apache/pinot/core/transport/TlsConfig.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvVGxzQ29uZmlnLmphdmE=) | `5.88% <0.00%> (+5.88%)` | :arrow_up: |
   | ... and [1003 more](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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/7540?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 [c8aa3e3...db05a8f](https://codecov.io/gh/apache/pinot/pull/7540?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 pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938190013


   Currently we return different values in the response metadata `ThreadCpuTimeNs` with the CPU thread time on/off, which is causing the confusion. Based on the discussion, I feel it is not a good idea to enable it by default. I'd suggest putting `-1` as `ThreadCpuTimeNs` if it is not enabled as a clear indication to the users that it is not enabled as @richardstartin suggested. What do you think? @siddharthteotia @mqliang 


-- 
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] siddharthteotia commented on a change in pull request #7540: Always enable the ThreadTimer

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



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -390,10 +390,6 @@ private CommonConstants() {
     public static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
         "org.apache.pinot.server.api.access.AllowAllAccessFactory";
 
-    public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =

Review comment:
       +1 on not deleting the config




-- 
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 #7540: Always enable the ThreadTimer

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


   @Jackie-Jiang 
   > Based on the discussion, I feel it is not a good idea to enable it by default. I'd suggest putting -1 as ThreadCpuTimeNs if it is not enabled as a clear indication to the users that it is not enabled as @richardstartin suggested. What do you think?
   
   I am not very concerned about the default value. I am more concerned about deleting the flag -- it's NOT a back compatible change, if user already start use the flag, after upgrading, pinot process will panic as user passed in a nonexistent flag.  
   
   Agree that disable by default as recording thread CPU time can be expensive.
   
   Agree that putting -1 or 0  as ThreadCpuTimeNs if it is not enabled.


-- 
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 pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938902920


   @mqliang Agree. I'm planning to merge #7544 and close this one, leave the default as disabled


-- 
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 pull request #7540: Always enable the ThreadTimer

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7540:
URL: https://github.com/apache/pinot/pull/7540#issuecomment-938277268


   Please take a look at #7544 @richardstartin @siddharthteotia @mqliang 


-- 
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 #7540: Always enable the ThreadTimer

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



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -390,10 +390,6 @@ private CommonConstants() {
     public static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
         "org.apache.pinot.server.api.access.AllowAllAccessFactory";
 
-    public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =

Review comment:
       Same here, I am OK with: `DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT = true;`. But don't delete it -- Let's allow user to opt-out ThreadTimer.

##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
##########
@@ -164,11 +163,6 @@ public void init(PinotConfiguration serverConf)
     // Initialize Pinot Environment Provider
     _pinotEnvironmentProvider = initializePinotEnvironmentProvider();
 
-    // Enable/disable thread CPU time measurement through instance config.

Review comment:
       Please don't delete this. I am OK with enabling ThreadTimer by default, but at least let's allow user to disable ThreadTimer.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/ThreadTimer.java
##########
@@ -28,42 +28,34 @@
  * The {@code ThreadTimer} class providing the functionality of measuring the CPU time for the current thread.
  */
 public class ThreadTimer {
+  private static final Logger LOGGER = LoggerFactory.getLogger(ThreadTimer.class);
   private static final ThreadMXBean MX_BEAN = ManagementFactory.getThreadMXBean();
   private static final boolean IS_CURRENT_THREAD_CPU_TIME_SUPPORTED = MX_BEAN.isCurrentThreadCpuTimeSupported();
-  private static final Logger LOGGER = LoggerFactory.getLogger(ThreadTimer.class);
-  private static boolean _isThreadCpuTimeMeasurementEnabled = false;
-  private long _startTimeNs = -1;
-  private long _endTimeNs = -1;
 
-  public ThreadTimer() {
-  }
+  private final long _startTimeNs;
 
-  public static void setThreadCpuTimeMeasurementEnabled(boolean enable) {
-    _isThreadCpuTimeMeasurementEnabled = enable && IS_CURRENT_THREAD_CPU_TIME_SUPPORTED;
-  }
-
-  public void start() {
-    if (_isThreadCpuTimeMeasurementEnabled) {
-      _startTimeNs = MX_BEAN.getCurrentThreadCpuTime();
-    }
-  }
-
-  public void stop() {
-    if (_isThreadCpuTimeMeasurementEnabled) {
-      _endTimeNs = MX_BEAN.getCurrentThreadCpuTime();
-    }
+  static {
+    LOGGER.info("Current thread cpu time measurement supported: {}", IS_CURRENT_THREAD_CPU_TIME_SUPPORTED);
   }
 
-  public long getThreadTimeNs() {
-    return _endTimeNs - _startTimeNs;
+  /**
+   * Constructs and starts the thread timer.
+   */
+  public ThreadTimer() {
+    _startTimeNs = getCurrentThreadCpuTime();
   }
 
+  /**
+   * Stops the thread timer and returns the thread CPU time in nanos.
+   */
   public long stopAndGetThreadTimeNs() {

Review comment:
       The function name `stopAndGetThreadTimeNs()` does not match the new logic, maybe change it as `getThreadTimeNsSinceStart()` or something similiar?




-- 
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 #7540: Always enable the ThreadTimer

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


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7540?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 [#7540](https://codecov.io/gh/apache/pinot/pull/7540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db05a8f) into [master](https://codecov.io/gh/apache/pinot/commit/c8aa3e39ccec1971dd71863c10b07254fe2cbc78?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8aa3e3) will **increase** coverage by `50.62%`.
   > The diff coverage is `83.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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    #7540       +/-   ##
   =============================================
   + Coverage     15.25%   65.88%   +50.62%     
   - Complexity       80     3414     +3334     
   =============================================
     Files          1477     1477               
     Lines         73824    73818        -6     
     Branches      10838    10840        +2     
   =============================================
   + Hits          11264    48637    +37373     
   + Misses        61753    21728    -40025     
   - Partials        807     3453     +2646     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests1 | `69.89% <83.33%> (?)` | |
   | unittests2 | `15.26% <0.00%> (+<0.01%)` | :arrow_up: |
   
   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/7540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/core/common/datatable/DataTableImplV3.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZUltcGxWMy5qYXZh) | `96.00% <ø> (+96.00%)` | :arrow_up: |
   | [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `78.08% <ø> (+78.08%)` | :arrow_up: |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `26.31% <ø> (+26.31%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7540/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==) | `90.00% <83.33%> (+90.00%)` | :arrow_up: |
   | [...lix/core/realtime/PinotRealtimeSegmentManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90UmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `75.64% <0.00%> (-4.67%)` | :arrow_down: |
   | [...controller/helix/core/minion/PinotTaskManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdFRhc2tNYW5hZ2VyLmphdmE=) | `42.14% <0.00%> (-1.92%)` | :arrow_down: |
   | [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `2.71% <0.00%> (+1.16%)` | :arrow_up: |
   | [.../segment/spi/index/reader/MutableForwardIndex.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3JlYWRlci9NdXRhYmxlRm9yd2FyZEluZGV4LmphdmE=) | `4.76% <0.00%> (+4.76%)` | :arrow_up: |
   | [...he/pinot/segment/local/utils/SegmentPushUtils.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9TZWdtZW50UHVzaFV0aWxzLmphdmE=) | `5.26% <0.00%> (+5.26%)` | :arrow_up: |
   | [...ava/org/apache/pinot/core/transport/TlsConfig.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvVGxzQ29uZmlnLmphdmE=) | `5.88% <0.00%> (+5.88%)` | :arrow_up: |
   | ... and [1003 more](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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/7540?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 [c8aa3e3...db05a8f](https://codecov.io/gh/apache/pinot/pull/7540?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] codecov-commenter edited a comment on pull request #7540: Always enable the ThreadTimer

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


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7540?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 [#7540](https://codecov.io/gh/apache/pinot/pull/7540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db05a8f) into [master](https://codecov.io/gh/apache/pinot/commit/c8aa3e39ccec1971dd71863c10b07254fe2cbc78?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8aa3e3) will **increase** coverage by `56.09%`.
   > The diff coverage is `83.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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    #7540       +/-   ##
   =============================================
   + Coverage     15.25%   71.35%   +56.09%     
   - Complexity       80     3414     +3334     
   =============================================
     Files          1477     1523       +46     
     Lines         73824    75667     +1843     
     Branches      10838    11040      +202     
   =============================================
   + Hits          11264    53989    +42725     
   + Misses        61753    17994    -43759     
   - Partials        807     3684     +2877     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration2 | `29.08% <83.33%> (?)` | |
   | unittests1 | `69.89% <83.33%> (?)` | |
   | unittests2 | `15.26% <0.00%> (+<0.01%)` | :arrow_up: |
   
   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/7540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/core/common/datatable/DataTableImplV3.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZUltcGxWMy5qYXZh) | `96.00% <ø> (+96.00%)` | :arrow_up: |
   | [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `78.08% <ø> (+78.08%)` | :arrow_up: |
   | [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `48.98% <ø> (ø)` | |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `26.31% <ø> (+26.31%)` | :arrow_up: |
   | [.../pinot/core/query/request/context/ThreadTimer.java](https://codecov.io/gh/apache/pinot/pull/7540/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==) | `90.00% <83.33%> (+90.00%)` | :arrow_up: |
   | [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `56.50% <0.00%> (ø)` | |
   | [...ugin/inputformat/csv/CSVRecordExtractorConfig.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtY3N2L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvY3N2L0NTVlJlY29yZEV4dHJhY3RvckNvbmZpZy5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...t/plugin/metrics/yammer/YammerMetricsRegistry.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3QtcGx1Z2lucy9waW5vdC1tZXRyaWNzL3Bpbm90LXlhbW1lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL21ldHJpY3MveWFtbWVyL1lhbW1lck1ldHJpY3NSZWdpc3RyeS5qYXZh) | `72.00% <0.00%> (ø)` | |
   | [...starter/helix/DefaultHelixStarterServerConfig.java](https://codecov.io/gh/apache/pinot/pull/7540/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9EZWZhdWx0SGVsaXhTdGFydGVyU2VydmVyQ29uZmlnLmphdmE=) | `100.00% <0.00%> (ø)` | |
   | ... and [1186 more](https://codecov.io/gh/apache/pinot/pull/7540/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/7540?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/7540?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 [c8aa3e3...db05a8f](https://codecov.io/gh/apache/pinot/pull/7540?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