kbuci commented on code in PR #10523:
URL: https://github.com/apache/hudi/pull/10523#discussion_r1456838438
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/LockManager.java:
##########
@@ -95,7 +97,10 @@ public void lock() {
*/
public void unlock() {
getLockProvider().unlock();
- metrics.updateLockHeldTimerMetrics();
+ if (lockAcquired) {
+ metrics.updateLockHeldTimerMetrics();
+ lockAcquired = false;
+ }
Review Comment:
Good point! The first fix idea I had originally was to have
`org.apache.hudi.common.util.HoodieTimer#endTimer` throw a different (sub)type
of HoodieException exception, and have LockManager::unlock() catch that (in
order to make sure that only this "timer not started" error would be
caught+logged). If I recalled correctly, I didn't do that route because I was
concerned that might cause other callers of
`org.apache.hudi.common.util.HoodieTimer` to break or need to be refactored.
Your suggestion here doesn't have that issue though. But my only concern then
is
- if having the logs of "Timer not started" might cause confusion for users
when they debug failed jobs (though hopefully the fact that the actual error
during tryLock will be bubbled up and logged will avoid any such confusion)?
- If there are other types of errors raised by
metrics.updateLockHeldTimerMetrics(); that we might want to have bubbled up in
the future (when I first made this PR I had assumed that, but now that I think
about it more I don't think this should be an issue)
If you don't think either of those are an issue, then we can go the route
you suggested.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
