Thanks Arpit Agarwal for working on this. The latest patch looks pretty good to me. Just have a few minor questions/issues below.
NIT: Can you update the comment (line 745, line 747) to reflect the changes of the
returned type? "FileInputStream" -> "FileDescriptor"
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.
NIT: Line 1033: BlockReceiver#adjustCrcFilePosition()
can we use streams.flushChecksumOut() here?
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.
NIT: Can you add a short description for the new key added or add cross reference to
the description in FileIoProvider class description.
NIT: these imports re-ordered with the imports below it
(only one added from this change though)
Line 1075: DatanodeUtil.dirNoFilesRecursive() can be wrapped into FileIoProvider.java to
get some aggregated metrics of dirNoFilesRecursive() in addition to FileIoProvider#listFiles().
Line: 202: this is a bug. We should delete the tmpFile instead of the file.
if (!fileIoProvider.delete(getVolume(), file))
Line 322,323: Should we close crcOut like blockOut and metataRAF here?
Can this be improved with a try-with-resource to avoid leaking.
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.
Should we count the number of errors in onFailure()?
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
, 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
Line 148: ReplicaOutputStreams#writeDataToDisk(), should we change
the dataOut/checksumOut to use the FileIoProvider#WrappedFileoutputStream
to get the FileIo write counted properly?
Line 83 readDataFully() should we change the dataIn/checksumIn
to use the FileIoProvider#WrappedFileInputStream to get the FileIo read counted properly?