Re: [PR] [HUDI-9159] S3 implementation of StorageLock for StorageBasedLockProvider [hudi]

["linliu-code (via GitHub)" <gi...@apache.org>]

linliu-code commented on code in PR #17955:
URL: https://github.com/apache/hudi/pull/17955#discussion_r2762084397


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/metrics/HoodieLockMetrics.java:
##########
@@ -91,10 +119,18 @@ public void startLockApiTimerContext() {
     }
   }
 
+  private static void updateMetric(HoodieTimer timer, Timer metric, String lockName) {

Review Comment:
   There is a bug in line 107 ~ 109.
   For example:
   lockDuration = createTimerForMetrics(registry, LOCK_ACQUIRE_DURATION_TIMER_NAME);
   lockApiRequestDuration = createTimerForMetrics(registry, LOCK_REQUEST_LATENCY_TIMER_NAME);
   the second call overwrites lockDuration with the request latency timer. Both lockDuration and lockApiRequestDuration end up referring to the same timer instance, while the registry still has a separate (and now unreferenced) timer for LOCK_ACQUIRE_DURATION. As a result, “lock held” duration is reported into the wrong metric.



-- 
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@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org