[GitHub] [pinot] mqliang commented on a change in pull request #7540: Always enable the ThreadTimer

[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