[ https://issues.apache.org/jira/browse/HDFS-10742?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15433853#comment-15433853 ]
Chris Douglas edited comment on HDFS-10742 at 8/23/16 11:28 PM: ---------------------------------------------------------------- * Is this the intended {{ConcurrentHashMap}} implementation? {noformat} +import org.jboss.netty.util.internal.ConcurrentHashMap; {noformat} The patch synchronizes on the instance, rather than using {{ConcurrentMap}} features. The tracking code could be simplified using those methods. * Some commented code (e.g., {{+ //Thread.dumpStack();}}) * Instead of adding the (unused) {{delay()}} method, consider passing in a {{org.apache.hadoop.util.Timer}} instance. This would also make it easier to write the unit test. * There's a comment explaining the importance of using thread-local storage, but the current patch neither uses nor requires it. * Please use the slf4j logger and vararg syntax instead of building strings that might be discarded/suppressed. * If the logger is null, shouldn't this fail? The {{Optional}} wrapper seems unnecessary. On a related note, the virtues of {{AutoCloseableLock}} (HADOOP-13466) are not obvious to me. Static analysis tools already catch what it enforces with {{AutoCloseable}}. Its interface is almost identical to {{java.util.concurrent.locks.Lock}} (admitting extensions like this). It seems to add an idiosyncrasy where there is already an idiom (with supporting tooling). Is there another advantage? was (Author: chris.douglas): * Is this the intended {{ConcurrentHashMap}} implementation? {noformat} +import org.jboss.netty.util.internal.ConcurrentHashMap; {noformat} The patch synchronizes on the instance, rather than using {{ConcurrentMap}} features. The tracking code could be simplified using those methods. * Some commented code (e.g., {{+ //Thread.dumpStack();}}) * Instead of adding the (unused) {{delay()}} method, consider passing in a {{org.apache.hadoop.util.Timer}} instance. This would also make it easier to write the unit test. * There's a comment explaining the importance of using thread-local storage, but the current patch neither uses nor requires it. * Please use the slf4j logger and vararg syntax instead of building strings that might be discarded/suppressed. * If the logger is null, shouldn't this fail? The {{Optional}} wrapper seems unnecessary. On a related note, the virtues of {{AutoCloseableLock}} (HADOOP-13466) are not obvious to me. Static analysis tools already catch what it enforces with {{AutoCloseable}}. Its interface is almost identical to {{java.util.concurrent.Lock}} (admitting extensions like this). It seems to add an idiosyncrasy where there is already an idiom (with supporting tooling). Is there another advantage? > Measurement of lock held time in FsDatasetImpl > ---------------------------------------------- > > Key: HDFS-10742 > URL: https://issues.apache.org/jira/browse/HDFS-10742 > Project: Hadoop HDFS > Issue Type: Improvement > Components: datanode > Affects Versions: 3.0.0-alpha2 > Reporter: Chen Liang > Assignee: Chen Liang > Attachments: HDFS-10742.001.patch, HDFS-10742.002.patch, > HDFS-10742.003.patch > > > This JIRA proposes to measure the time the of lock of {{FsDatasetImpl}} is > held by a thread. Doing so will allow us to measure lock statistics. > This can be done by extending the {{AutoCloseableLock}} lock object in > {{FsDatasetImpl}}. In the future we can also consider replacing the lock with > a read-write lock. -- 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