[
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: [email protected]
For additional commands, e-mail: [email protected]
