hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dennis Huo (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-13056) Expose file-level composite CRCs in HDFS which are comparable across different instances/layouts
Date Fri, 23 Mar 2018 01:06:00 GMT

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

Dennis Huo commented on HDFS-13056:
-----------------------------------

Thanks for the review [~xiaochen]! Responses:

 
 - What's the plan for webhdfs? It seems {{DFSClient#getFileChecksum}} is only necessary
because of webhdfs.

I figured we might want to keep DFSClient#getFileChecksum around in case some users are calling
directly into the java-public interfaces of DFSClient despite the class being annotated as
private, since it's one of the helper classes close enough to the real public interfaces that
it's more likely than, say, the FileChecksumHelper to have made it into some people's code.

But that was just because I'm not too familiar with the overall policies on breaking java-public
interfaces for classes that are marked with private InterfaceAudience. I'll be happy to try
to remove the MD5-specific getFileChecksum method entirely from DFSClient if you think it's
safe.

It looks like a few files would need to be touched to finish plumbing the MD5MD5CRC32FileChecksum
assumption out, including JsonUtil; let me know if you think it's better to go ahead and
finish the WebHDFS plumbing in this patch, or to do it in a separate Jira as a followup.
 - {{InterfaceAudience.Private}} is by default {{InterfaceStability.Unstable}}, so we can
probably remove {{Evolving}} unless there is a reason. We should mark the {{LimitedPrivate}} classes
to be unstable instead of evolving too.

Done
 - The {{StripedBlockChecksumReconstructor}} refactor into the prepareDigester/updateDigester/commitDigest
looks a little odd to me. But I understand if not doing it this way, we'd need to implement
2 mostly-duplicated {{reconstruct}}. At the minimum we should check for {{digester != null}} in
the latter 2 methods.

Done
 - CrcUtil: I see {{writeInt}} and {{readInt}} mentions the requirement about buffer length
v.s. offset, which is good. But for code safety, let's add validation to actually check the
input parameters.

Done
 - CrcUtil: newSingleCrcWrapperFromByteArray: how about naming it to something like toSingleCrcString
and a {{String}} instead of an {{Object}}? In the caller we can also save it to a String
variable (and similarly save md5.toString to it, within an isDebugEnabled check).

Done. The original purpose was to keep it more analogous to the md5 case which relies on the
late evaluation of debug params and thus avoid having to check isDebugEnabled, but as you noted,
I added the isDebugEnabled checks anyways in response to previous review comments in this
Jira ([https://github.com/apache/hadoop/pull/344/commits/1bb2260805c4ecdf014422d2d0eebafe81973a53)],
and I agree it's a bit clunky to have this "Object" wrapper hack. Went ahead and switched
to returning Strings directly since it's easier to read.
 - FileChecksumHelper: {{makeFinalResult}} returns null instead of throw when {{combineMode}} is
unknown, unlike other places. I think throw is the right thing to do.

Done
 - FileChecksumHelper: {{tryDatanode}} Let's also log the {{blockChecksumType}} in the
debug log in the end of the method

Done
 - BlockChecksumOptions: trivial, more canonical to call the 2-param ctor from the 1-param
ctor, like {{this(blockChecksumType, 0);}}

Done
 - LOG.isDebugEnabled is not required if using parameterized messages: [https://www.slf4j.org/faq.html#logging_performance] So
can remove from {{BlockChecksumHelper$BlockGroupNonStripedChecksumComputer#reassembleNonStripedCompositeCrc}}

Since I switched to CrcUtil.toSingleCrcString as suggested, I kept the isDebugEnabled checks
around the calls instead of relying on late evaluation of stringifying the crcs.

> Expose file-level composite CRCs in HDFS which are comparable across different instances/layouts
> ------------------------------------------------------------------------------------------------
>
>                 Key: HDFS-13056
>                 URL: https://issues.apache.org/jira/browse/HDFS-13056
>             Project: Hadoop HDFS
>          Issue Type: New Feature
>          Components: datanode, distcp, erasure-coding, federation, hdfs
>    Affects Versions: 3.0.0
>            Reporter: Dennis Huo
>            Assignee: Dennis Huo
>            Priority: Major
>         Attachments: HDFS-13056-branch-2.8.001.patch, HDFS-13056-branch-2.8.002.patch,
HDFS-13056-branch-2.8.003.patch, HDFS-13056-branch-2.8.004.patch, HDFS-13056-branch-2.8.005.patch,
HDFS-13056-branch-2.8.poc1.patch, HDFS-13056.001.patch, HDFS-13056.002.patch, HDFS-13056.003.patch,
HDFS-13056.003.patch, HDFS-13056.004.patch, HDFS-13056.005.patch, HDFS-13056.006.patch, HDFS-13056.007.patch,
HDFS-13056.008.patch, HDFS-13056.009.patch, Reference_only_zhen_PPOC_hadoop2.6.X.diff, hdfs-file-composite-crc32-v1.pdf,
hdfs-file-composite-crc32-v2.pdf, hdfs-file-composite-crc32-v3.pdf
>
>
> FileChecksum was first introduced in [https://issues-test.apache.org/jira/browse/HADOOP-3981] and
ever since then has remained defined as MD5-of-MD5-of-CRC, where per-512-byte chunk CRCs are
already stored as part of datanode metadata, and the MD5 approach is used to compute an aggregate
value in a distributed manner, with individual datanodes computing the MD5-of-CRCs per-block
in parallel, and the HDFS client computing the second-level MD5.
>  
> A shortcoming of this approach which is often brought up is the fact that this FileChecksum
is sensitive to the internal block-size and chunk-size configuration, and thus different
HDFS files with different block/chunk settings cannot be compared. More commonly, one might
have different HDFS clusters which use different block sizes, in which case any data migration
won't be able to use the FileChecksum for distcp's rsync functionality or for verifying end-to-end
data integrity (on top of low-level data integrity checks applied at data transfer time).
>  
> This was also revisited in https://issues.apache.org/jira/browse/HDFS-8430 during the
addition of checksum support for striped erasure-coded files; while there was some discussion
of using CRC composability, it still ultimately settled on hierarchical MD5 approach, which also adds
the problem that checksums of basic replicated files are not comparable to striped files.
>  
> This feature proposes to add a "COMPOSITE-CRC" FileChecksum type which uses CRC composition
to remain completely chunk/block agnostic, and allows comparison between striped vs replicated
files, between different HDFS instances, and possible even between HDFS and other external
storage systems. This feature can also be added in-place to be compatible with existing block
metadata, and doesn't need to change the normal path of chunk verification, so is minimally
invasive. This also means even large preexisting HDFS deployments could adopt this feature
to retroactively sync data. A detailed design document can be found here: https://storage.googleapis.com/dennishuo/hdfs-file-composite-crc32-v1.pdf



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
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