[ https://issues.apache.org/jira/browse/HDFS-10924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15556560#comment-15556560 ]
Xiao Chen commented on HDFS-10924: ---------------------------------- Thanks [~jingcheng...@intel.com] for the nice work here, and on the related HDFS-9668. Some comments on patch 3, looks good overall: {{AutoCloseableReadWriteLockWrapper}}: - Are the constructors of {{AutoCloseableReadLock}} and {{AutoCloseableWriteLock}} intentionally public? I'm having a hard time coming up with a scenario of this. - Maybe we could update the above to constructors to accept the {{lock}} parameter. This is also more consistent with what the new instrumented lock class does. (as well as java's {{ReentrantReadWriteLock}} class) {{InstrumentedAutoCloseableReadWriteLockWrapper}}: - Please check input variables on {{InstrumentedAutoCloseableReadWriteLockWrapper}}'s constructor. (e.g. time > 0, logger not null. Do we allow null name?) - Personally feels the following change would be more readable: {code} if (lockWarningThresholdMs - lockHeldTime < 0) { ... now = clock.monotonicNow(); localLastLogTs = lastLogTimestamp.get(); long deltaSinceLastLog = now - localLastLogTs; // check should print log or not if (deltaSinceLastLog - minLoggingGapMs < 0) { warningsSuppressed.incrementAndGet(); return; } {code} to {code} if (lockHeldTime > lockWarningThresholdMs) { .... now = clock.monotonicNow(); localLastLogTs = lastLogTimestamp.get(); if (now - localLastLogTs < minLoggingGapMs) { warningsSuppressed.incrementAndGet(); return; } {code} - Feels like a StringBuilder would be more readable and efficient in {{logWarning}}, than the current 3 concatenation. > Add a new instrumented read-write lock > -------------------------------------- > > Key: HDFS-10924 > URL: https://issues.apache.org/jira/browse/HDFS-10924 > Project: Hadoop HDFS > Issue Type: Improvement > Components: datanode > Reporter: Jingcheng Du > Assignee: Jingcheng Du > Attachments: HDFS-10924-2.patch, HDFS-10924-3.patch, HDFS-10924.patch > > > Add a new instrumented read-write lock in hadoop common, so that the > HDFS-9668 can use this to improve the locking in FsDatasetImpl -- This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org