hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiaoyu Yao (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-10958) Add instrumentation hooks around Datanode disk IO
Date Mon, 12 Dec 2016 22:53:58 GMT

    [ https://issues.apache.org/jira/browse/HDFS-10958?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15743429#comment-15743429
] 

Xiaoyu Yao commented on HDFS-10958:
-----------------------------------

Thanks [~arpitagarwal] for working on this. The latest patch looks pretty good to me. Just
have a few minor questions/issues below.

1. NatievIO.java#getShareDeleteFileDescriptor
NIT: Can you update the comment (line 745, line 747) to reflect the changes of the
returned type? "FileInputStream" -> "FileDescriptor"

2. BlockMetadataHeader.java
Line 149: BlockMetadataHeader#readHeader(File file) can be removed

Line 85: From the caller of BlockMetadataHeader#readDataChecksum() in 
FsDatasetImpl#computeChecksum, we can get a hook for FileInputStream. Is it possible
to add hook for readDataCheckum into FileIoProvider or a WrappedFileInputStream
for measurement of the reading performance.

3. BlockReceiver.java
NIT: Line 1033: BlockReceiver#adjustCrcFilePosition() 
can we use streams.flushChecksumOut() here?

4. DatanodeUtil.java
NIT: Line 59: Can we move DatanodeUtil#createFileWithExistsCheck to FileIoProvider like
we do for mkdirsWithExistsCheck/deleteWithExistsCheck?

Line 1365: DataStorage#fullyDelete(). I'm OK with deprecate it.
There seems to be no reference to this method. So maybe we can remove it.

5. DFSConfigKeys.java
NIT: Can you add a short description for the new key added or add cross reference to
the description in FileIoProvider class description.

6. FsDatasetImpl.java
NIT: these imports re-ordered with the imports below it
(only one added from this change though)
import org.apache.hadoop.hdfs.DFSConfigKeys;
import org.apache.hadoop.hdfs.DFSUtilClient;
import org.apache.hadoop.hdfs.ExtendedBlockId;
import org.apache.hadoop.hdfs.server.datanode.FileIoProvider;
import org.apache.hadoop.util.AutoCloseableLock;

7. FSVolumeImpl.java
Line 1075: DatanodeUtil.dirNoFilesRecursive() can be wrapped into FileIoProvider.java to
get some aggregated metrics of dirNoFilesRecursive() in addition to FileIoProvider#listFiles().

 8. LocalReplica.java
Line: 202: this is a bug. We should delete the tmpFile instead of the file.
{code}
if (!fileIoProvider.delete(getVolume(), file)) 
{code}

9. LocalReplicaInPipeline.java
Line 322,323: Should we close crcOut like blockOut and metataRAF here? 
Can this be improved with a try-with-resource to avoid leaking.

10. FileIoEvents.java
Line 89: FileIoEvents#onFailure() can we add a begin parameter for the failure 
code path so that we can track the time spent on FileIo/Metadata before failure.

11. CountingFileIoEvents.java
Should we count the number of errors in onFailure()? 

12. FileIoProvider.java
NIT: some of the methods are missing Javadocs for the last few added 
@param such as flush()/listDirectory()/linkCount()/mkdirs, etc.

Line 105: NIT: We can add a tag to the enum FileIoProvider#OPERATION to explicitly
describe the operation type FileIo/Metadata, which could simplify the FileIoEvents interface.

I'm OK with the current implementation, which is also good and easy to follow. 

Line 155: I think we should put sync() under fileIo op instead of metadata op based
on we are passing true to {code}fos.getChannel().force(true);{code}, which force
both metadata and data written on device.

Line 459: FileIoProvider#fullyDelete() should we declare exception just for fault
injection purpose? FileUtil.fullyDelete() itself does not throw. 

Line 575: NIT: File f -> File dir
Line 598: NIT: File f -> File dir

13. ReplicaOutputStreams.java
Line 148: ReplicaOutputStreams#writeDataToDisk(), should we change 
the dataOut/checksumOut to use the FileIoProvider#WrappedFileoutputStream 
to get the FileIo write counted properly?

14. ReplicaInputStreams.java
Line 83 readDataFully() should we change the dataIn/checksumIn 
to use the FileIoProvider#WrappedFileInputStream to get the FileIo read counted properly?





> Add instrumentation hooks around Datanode disk IO
> -------------------------------------------------
>
>                 Key: HDFS-10958
>                 URL: https://issues.apache.org/jira/browse/HDFS-10958
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>          Components: datanode
>            Reporter: Xiaoyu Yao
>            Assignee: Arpit Agarwal
>         Attachments: HDFS-10958.01.patch, HDFS-10958.02.patch, HDFS-10958.03.patch, HDFS-10958.04.patch
>
>
> This ticket is opened to add instrumentation hooks around Datanode disk IO based on refactor
work from HDFS-10930.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message