[
https://issues.apache.org/jira/browse/HBASE-5074?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13198280#comment-13198280
]
Phabricator commented on HBASE-5074:
------------------------------------
todd has commented on the revision "[jira] [HBASE-5074] Support checksums in
HBase block cache".
I'm a little skeptical of pushing HFileSystem in at the createHRegion level -
can't we construct it lower down and have fewer sweeping changes across the
codebase?
Otherwise I'm pretty psyched about this feature! Should be a great speed
boost.
INLINE COMMENTS
src/main/java/org/apache/hadoop/hbase/HConstants.java:598 I think this could
be clarified a bit... I am at the top of the diff so don't have context, so not
sure whether it means that _no_ checksums will be verified, or if checksums
will be verified but only when the HFile checksum isn't present or can't be
verified?
I'd expect the config to have several different modes, rather than a boolean:
FS_ONLY: always verify the FS checksum, ignore the HFile checksum
BOTH: always verify the FS checksum and the HFile checksum (when available)
OPTIMIZED: verify the HFile checksum. If it fails or not present, fall back
to the FS checksum (this would be the default)
NONE: don't verify any checksums (for those who like to live on the edge!)
src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java:357
This is sort of working around a deficiency in the Hadoop input stream APIs,
right? I think this is a decent workaround for now, but do you think it would
be a good idea to add a new interface like "Checksummed" to Hadoop, which would
add a setVerifyChecksum() call?
src/main/java/org/apache/hadoop/hbase/io/hfile/FixedFileTrailer.java:123-127
do we need this extra ctor? considering this is a private API seems like we
could just update the call sites to add a ', 0'
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:93 may be
worth adding a constant here like VERSION_CURRENT = VERSION_WITH_CHECKSUM.
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:102 or
HEADER_SIZE_V1 = ...
static final int HEADER_SIZE = HEADER_SIZE_V1;
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:132 why is
this a warning?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:175 this takes
a parameter minorVersion - is it unused?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:190 typo:
@param minorVersion
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:205-208
confused about this - see above... if this constructor is only meant for minor
version 0 blocks, shouldn't we have Preconditions.checkArgument(minorVersion ==
0) or something?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:229 Skeptical
of this line -- why isn't it onDiskDataSizeWithoutHeader + HEADER_SIZE_V0?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:252 a little
confused why this doesn't use the onDiskDataSizeWithHeader member...
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:629 typo:
incudes
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:751 It would
be nice to reuse the code in Hadoop trunk here - see
DataChecksum.verifyChunkedSums for example. The benefit is that we have JNI
implementations using SSE code there. Only downside is that the JNI code
requires direct byte buffers, which I guess we aren't using here... perhaps
punt to a future improvement.
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:774 I assume
this will move to a trace level debug or something?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1634 I think
rather than returning null it makes more sense to throw a ChecksumException here
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1401-1402 I
don't know this area of the code well -- is it supposed to be thread-safe? This
lazy-initialization pattern is not.
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1605 can we
just recurse here?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1896 it's not
possible to get at the file path from this context, is it?
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1888 would be
nice to reuse Hadoop code here if possible for performance
src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java:206-207
this might result in "Wrong FS" if the default FS doesn't match the path
provided
src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java:1072 can
you include the path in the msg?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3546-3547
this code should be passing the path as well to avoid "Wrong FS"
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3597 same -
default FS may not match the hbase rootdir FS. Maybe RegionServerServices
should expose a getHFilesystem() call?
src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java:335
this downcast would be avoided by adding getFileSystem to RegionServerServices
above
src/main/java/org/apache/hadoop/hbase/util/ChecksumByteArrayOutputStream.java:31
could you reuse o.a.h.io.DataOutputBuffer instead of making a new class?
src/main/java/org/apache/hadoop/hbase/util/ChecksumFactory.java:39 I'd
strongly recommend starting with an implementation of CRC32C (castagnioli
polynomial) instead of the Zip polynomial - reason being that there is a
hardware implementation in SSE4.2. You can rip the pure-java implementation
from Hadoop trunk (PureJavaCrc32C) into HBase.
Failing that, we need to add hfile metadata which specifies the checksum
algorithm in addition to the checksum type.
You could reuse the DataChecksum class from Hadoop there - it encapsulates
(type, bytesPerChecksum, checksumSize)
src/main/java/org/apache/hadoop/hbase/util/ChecksumFactory.java:65-67 this
reflection based method is going to be horribly slow
src/main/java/org/apache/hadoop/hbase/util/HFileSystem.java:59 typo,
operation.
src/main/java/org/apache/hadoop/hbase/util/HFileSystem.java:54 the ctor
should take a Path or URI indicating the filesystem, rather than always using
the default - same "wrong fs" issue as above
src/main/java/org/apache/hadoop/hbase/util/HFileSystem.java:67 typo: in->is
src/main/java/org/apache/hadoop/hbase/util/HFileSystem.java:132-133 this is
rarely the right call
REVISION DETAIL
https://reviews.facebook.net/D1521
> support checksums in HBase block cache
> --------------------------------------
>
> Key: HBASE-5074
> URL: https://issues.apache.org/jira/browse/HBASE-5074
> Project: HBase
> Issue Type: Improvement
> Components: regionserver
> Reporter: dhruba borthakur
> Assignee: dhruba borthakur
> Attachments: D1521.1.patch, D1521.1.patch
>
>
> The current implementation of HDFS stores the data in one block file and the
> metadata(checksum) in another block file. This means that every read into the
> HBase block cache actually consumes two disk iops, one to the datafile and
> one to the checksum file. This is a major problem for scaling HBase, because
> HBase is usually bottlenecked on the number of random disk iops that the
> storage-hardware offers.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
