diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java index c84836bcd532..7da114f14b0e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java @@ -172,7 +172,7 @@ static class Header { * {@link #onDiskSizeWithoutHeader} when using HDFS checksum. * @see Writer#putHeader(byte[], int, int, int, int) */ - private int onDiskDataSizeWithHeader; + private final int onDiskDataSizeWithHeader; // End of Block Header fields. /** @@ -188,13 +188,15 @@ static class Header { * ByteBuffer-like API across multiple ByteBuffers reading from a cache such as BucketCache. So, * we have this ByteBuff type. Unfortunately, it is spread all about HFileBlock. Would be good if * could be confined to cache-use only but hard-to-do. + *

+ * NOTE: this byteBuff including HFileBlock header and data, but excluding checksum. */ - private ByteBuff buf; + private ByteBuff bufWithoutChecksum; /** * Meta data that holds meta information on the hfileblock. */ - private HFileContext fileContext; + private final HFileContext fileContext; /** * The offset of this block in the file. Populated by the reader for convenience of access. This @@ -296,6 +298,8 @@ public int getDeserializerIdentifier() { CacheableDeserializerIdManager.registerDeserializer(BLOCK_DESERIALIZER); } + private final int totalChecksumBytes; + /** * Creates a new {@link HFile} block from the given fields. This constructor is used only while * writing blocks and caching, and is sitting in a byte buffer and we want to stuff the block into @@ -332,11 +336,12 @@ public HFileBlock(BlockType blockType, int onDiskSizeWithoutHeader, this.nextBlockOnDiskSize = nextBlockOnDiskSize; this.fileContext = fileContext; this.allocator = allocator; - this.buf = buf; + this.bufWithoutChecksum = buf; if (fillHeader) { overwriteHeader(); } - this.buf.rewind(); + this.bufWithoutChecksum.rewind(); + this.totalChecksumBytes = computeTotalChecksumBytes(); } /** @@ -411,12 +416,12 @@ public BlockType getBlockType() { @Override public int refCnt() { - return buf.refCnt(); + return bufWithoutChecksum.refCnt(); } @Override public HFileBlock retain() { - buf.retain(); + bufWithoutChecksum.retain(); return this; } @@ -426,7 +431,7 @@ public HFileBlock retain() { */ @Override public boolean release() { - return buf.release(); + return bufWithoutChecksum.release(); } /** @@ -441,7 +446,7 @@ public HFileBlock touch() { @Override public HFileBlock touch(Object hint) { - buf.touch(hint); + bufWithoutChecksum.touch(hint); return this; } @@ -451,7 +456,7 @@ short getDataBlockEncodingId() { throw new IllegalArgumentException("Querying encoder ID of a block " + "of type other than " + BlockType.ENCODED_DATA + ": " + blockType); } - return buf.getShort(headerSize()); + return bufWithoutChecksum.getShort(headerSize()); } /** Returns the on-disk size of header + data part + checksum. */ @@ -479,15 +484,15 @@ long getPrevBlockOffset() { * side-effect. */ private void overwriteHeader() { - buf.rewind(); - blockType.write(buf); - buf.putInt(onDiskSizeWithoutHeader); - buf.putInt(uncompressedSizeWithoutHeader); - buf.putLong(prevBlockOffset); + bufWithoutChecksum.rewind(); + blockType.write(bufWithoutChecksum); + bufWithoutChecksum.putInt(onDiskSizeWithoutHeader); + bufWithoutChecksum.putInt(uncompressedSizeWithoutHeader); + bufWithoutChecksum.putLong(prevBlockOffset); if (this.fileContext.isUseHBaseChecksum()) { - buf.put(fileContext.getChecksumType().getCode()); - buf.putInt(fileContext.getBytesPerChecksum()); - buf.putInt(onDiskDataSizeWithHeader); + bufWithoutChecksum.put(fileContext.getChecksumType().getCode()); + bufWithoutChecksum.putInt(fileContext.getBytesPerChecksum()); + bufWithoutChecksum.putInt(onDiskDataSizeWithHeader); } } @@ -507,11 +512,12 @@ public ByteBuff getBufferWithoutHeader() { * in {@link CompoundBloomFilter} to avoid object creation on every Bloom filter lookup, but has * to be used with caution. Buffer holds header, block content, and any follow-on checksums if * present. - * @return the buffer of this block for read-only operations + * @return the buffer of this block for read-only operations,the buffer includes header,but not + * checksum. */ public ByteBuff getBufferReadOnly() { // TODO: ByteBuf does not support asReadOnlyBuffer(). Fix. - ByteBuff dup = this.buf.duplicate(); + ByteBuff dup = this.bufWithoutChecksum.duplicate(); assert dup.position() == 0; return dup; } @@ -545,7 +551,7 @@ private void sanityCheckAssertion(BlockType valueFromBuf, BlockType valueFromFie */ void sanityCheck() throws IOException { // Duplicate so no side-effects - ByteBuff dup = this.buf.duplicate().rewind(); + ByteBuff dup = this.bufWithoutChecksum.duplicate().rewind(); sanityCheckAssertion(BlockType.read(dup), blockType); sanityCheckAssertion(dup.getInt(), onDiskSizeWithoutHeader, "onDiskSizeWithoutHeader"); @@ -588,8 +594,8 @@ public String toString() { .append(", prevBlockOffset=").append(prevBlockOffset).append(", isUseHBaseChecksum=") .append(fileContext.isUseHBaseChecksum()); if (fileContext.isUseHBaseChecksum()) { - sb.append(", checksumType=").append(ChecksumType.codeToType(this.buf.get(24))) - .append(", bytesPerChecksum=").append(this.buf.getInt(24 + 1)) + sb.append(", checksumType=").append(ChecksumType.codeToType(this.bufWithoutChecksum.get(24))) + .append(", bytesPerChecksum=").append(this.bufWithoutChecksum.getInt(24 + 1)) .append(", onDiskDataSizeWithHeader=").append(onDiskDataSizeWithHeader); } else { sb.append(", onDiskDataSizeWithHeader=").append(onDiskDataSizeWithHeader).append("(") @@ -597,9 +603,10 @@ public String toString() { .append(HConstants.HFILEBLOCK_HEADER_SIZE_NO_CHECKSUM).append(")"); } String dataBegin; - if (buf.hasArray()) { - dataBegin = Bytes.toStringBinary(buf.array(), buf.arrayOffset() + headerSize(), - Math.min(32, buf.limit() - buf.arrayOffset() - headerSize())); + if (bufWithoutChecksum.hasArray()) { + dataBegin = Bytes.toStringBinary(bufWithoutChecksum.array(), + bufWithoutChecksum.arrayOffset() + headerSize(), + Math.min(32, bufWithoutChecksum.limit() - bufWithoutChecksum.arrayOffset() - headerSize())); } else { ByteBuff bufWithoutHeader = getBufferWithoutHeader(); byte[] dataBeginBytes = @@ -609,8 +616,8 @@ public String toString() { } sb.append(", getOnDiskSizeWithHeader=").append(getOnDiskSizeWithHeader()) .append(", totalChecksumBytes=").append(totalChecksumBytes()).append(", isUnpacked=") - .append(isUnpacked()).append(", buf=[").append(buf).append("]").append(", dataBeginsWith=") - .append(dataBegin).append(", fileContext=").append(fileContext) + .append(isUnpacked()).append(", buf=[").append(bufWithoutChecksum).append("]") + .append(", dataBeginsWith=").append(dataBegin).append(", fileContext=").append(fileContext) .append(", nextBlockOnDiskSize=").append(nextBlockOnDiskSize).append("]"); return sb.toString(); } @@ -639,7 +646,7 @@ HFileBlock unpack(HFileContext fileContext, FSReader reader) throws IOException : reader.getDefaultBlockDecodingContext(); // Create a duplicated buffer without the header part. int headerSize = this.headerSize(); - ByteBuff dup = this.buf.duplicate(); + ByteBuff dup = this.bufWithoutChecksum.duplicate(); dup.position(headerSize); dup = dup.slice(); // Decode the dup into unpacked#buf @@ -662,7 +669,7 @@ private ByteBuff allocateBufferForUnpacking() { int headerSize = headerSize(); int capacityNeeded = headerSize + uncompressedSizeWithoutHeader; - ByteBuff source = buf.duplicate(); + ByteBuff source = bufWithoutChecksum.duplicate(); ByteBuff newBuf = allocator.allocate(capacityNeeded); // Copy header bytes into newBuf. @@ -681,7 +688,7 @@ private ByteBuff allocateBufferForUnpacking() { public boolean isUnpacked() { final int headerSize = headerSize(); final int expectedCapacity = headerSize + uncompressedSizeWithoutHeader; - final int bufCapacity = buf.remaining(); + final int bufCapacity = bufWithoutChecksum.remaining(); return bufCapacity == expectedCapacity || bufCapacity == expectedCapacity + headerSize; } @@ -697,9 +704,9 @@ long getOffset() { return offset; } - /** Returns a byte stream reading the data + checksum of this block */ + /** Returns a byte stream reading the data(excluding header and checksum) of this block */ DataInputStream getByteStream() { - ByteBuff dup = this.buf.duplicate(); + ByteBuff dup = this.bufWithoutChecksum.duplicate(); dup.position(this.headerSize()); return new DataInputStream(new ByteBuffInputStream(dup)); } @@ -708,9 +715,9 @@ DataInputStream getByteStream() { public long heapSize() { long size = FIXED_OVERHEAD; size += fileContext.heapSize(); - if (buf != null) { + if (bufWithoutChecksum != null) { // Deep overhead of the byte buffer. Needs to be aligned separately. - size += ClassSize.align(buf.capacity() + MULTI_BYTE_BUFFER_HEAP_SIZE); + size += ClassSize.align(bufWithoutChecksum.capacity() + MULTI_BYTE_BUFFER_HEAP_SIZE); } return ClassSize.align(size); } @@ -1848,9 +1855,9 @@ void sanityCheckUncompressed() throws IOException { // Cacheable implementation @Override public int getSerializedLength() { - if (buf != null) { + if (bufWithoutChecksum != null) { // Include extra bytes for block metadata. - return this.buf.limit() + BLOCK_METADATA_SPACE; + return this.bufWithoutChecksum.limit() + BLOCK_METADATA_SPACE; } return 0; } @@ -1858,7 +1865,7 @@ public int getSerializedLength() { // Cacheable implementation @Override public void serialize(ByteBuffer destination, boolean includeNextBlockMetadata) { - this.buf.get(destination, 0, getSerializedLength() - BLOCK_METADATA_SPACE); + this.bufWithoutChecksum.get(destination, 0, getSerializedLength() - BLOCK_METADATA_SPACE); destination = addMetaData(destination, includeNextBlockMetadata); // Make it ready for reading. flip sets position to zero and limit to current position which @@ -1904,7 +1911,7 @@ public int hashCode() { result = result * 31 + onDiskSizeWithoutHeader; result = result * 31 + (int) (prevBlockOffset ^ (prevBlockOffset >>> 32)); result = result * 31 + uncompressedSizeWithoutHeader; - result = result * 31 + buf.hashCode(); + result = result * 31 + bufWithoutChecksum.hashCode(); return result; } @@ -1942,8 +1949,8 @@ public boolean equals(Object comparison) { return false; } if ( - ByteBuff.compareTo(this.buf, 0, this.buf.limit(), castedComparison.buf, 0, - castedComparison.buf.limit()) != 0 + ByteBuff.compareTo(this.bufWithoutChecksum, 0, this.bufWithoutChecksum.limit(), + castedComparison.bufWithoutChecksum, 0, castedComparison.bufWithoutChecksum.limit()) != 0 ) { return false; } @@ -1971,10 +1978,17 @@ int getOnDiskDataSizeWithHeader() { } /** - * Calculate the number of bytes required to store all the checksums for this block. Each checksum - * value is a 4 byte integer. + * Return the number of bytes required to store all the checksums for this block. Each checksum + * value is a 4 byte integer.
+ * NOTE: ByteBuff returned by {@link HFileBlock#getBufferWithoutHeader()} and + * {@link HFileBlock#getBufferReadOnly} or DataInputStream returned by + * {@link HFileBlock#getByteStream()} does not include checksum. */ int totalChecksumBytes() { + return totalChecksumBytes; + } + + private int computeTotalChecksumBytes() { // If the hfile block has minorVersion 0, then there are no checksum // data to validate. Similarly, a zero value in this.bytesPerChecksum // indicates that cached blocks do not have checksum data because @@ -2071,7 +2085,8 @@ private static HFileBlock shallowClone(HFileBlock blk, ByteBuff newBuf) { } static HFileBlock deepCloneOnHeap(HFileBlock blk) { - ByteBuff deepCloned = ByteBuff.wrap(ByteBuffer.wrap(blk.buf.toBytes(0, blk.buf.limit()))); + ByteBuff deepCloned = ByteBuff + .wrap(ByteBuffer.wrap(blk.bufWithoutChecksum.toBytes(0, blk.bufWithoutChecksum.limit()))); return createBuilder(blk, deepCloned).build(); } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java index b5a5095c3367..12ef197af439 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlockIndex.java @@ -883,10 +883,10 @@ public DataInputStream readRootIndex(HFileBlock blk, final int numEntries) throw */ public void readMultiLevelIndexRoot(HFileBlock blk, final int numEntries) throws IOException { DataInputStream in = readRootIndex(blk, numEntries); - // after reading the root index the checksum bytes have to - // be subtracted to know if the mid key exists. - int checkSumBytes = blk.totalChecksumBytes(); - if ((in.available() - checkSumBytes) < MID_KEY_METADATA_SIZE) { + // HFileBlock.getByteStream() returns a byte stream for reading the data(excluding checksum) + // of root index block, so after reading the root index there is no need to subtract the + // checksum bytes. + if (in.available() < MID_KEY_METADATA_SIZE) { // No mid-key metadata available. return; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpIndexBlockEncoder.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpIndexBlockEncoder.java index 9e480247ee9a..3115a5153c21 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpIndexBlockEncoder.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpIndexBlockEncoder.java @@ -204,10 +204,10 @@ public void initRootIndex(HFileBlock blk, int numEntries, CellComparator compara private void init(HFileBlock blk, int numEntries) throws IOException { DataInputStream in = readRootIndex(blk, numEntries); - // after reading the root index the checksum bytes have to - // be subtracted to know if the mid key exists. - int checkSumBytes = blk.totalChecksumBytes(); - if ((in.available() - checkSumBytes) < MID_KEY_METADATA_SIZE) { + // HFileBlock.getByteStream() returns a byte stream for reading the data(excluding checksum) + // of root index block, so after reading the root index there is no need to subtract the + // checksum bytes. + if (in.available() < MID_KEY_METADATA_SIZE) { // No mid-key metadata available. return; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java index 5b8cfadfde78..1c158ccdf3b8 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue; import java.io.ByteArrayOutputStream; +import java.io.DataOutput; import java.io.DataOutputStream; import java.io.IOException; import java.nio.ByteBuffer; @@ -50,10 +51,14 @@ import org.apache.hadoop.hbase.PrivateCellUtil; import org.apache.hadoop.hbase.fs.HFileSystem; import org.apache.hadoop.hbase.io.ByteBuffAllocator; +import org.apache.hadoop.hbase.io.FSDataInputStreamWrapper; import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.compress.Compression.Algorithm; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; +import org.apache.hadoop.hbase.io.encoding.IndexBlockEncoding; +import org.apache.hadoop.hbase.io.hfile.HFile.Writer; import org.apache.hadoop.hbase.io.hfile.HFileBlockIndex.BlockIndexReader; +import org.apache.hadoop.hbase.io.hfile.NoOpIndexBlockEncoder.NoOpEncodedSeeker; import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.nio.MultiByteBuff; import org.apache.hadoop.hbase.testclassification.IOTests; @@ -740,4 +745,96 @@ public void testIntermediateLevelIndicesWithLargeKeys(int minNumEntries) throws } reader.close(); } + + /** + * This test is for HBASE-27940, which midkey metadata in root index block would always be ignored + * by {@link BlockIndexReader#readMultiLevelIndexRoot}. + */ + @Test + public void testMidKeyReadSuccessfullyFromRootIndexBlock() throws IOException { + conf.setInt(HFileBlockIndex.MAX_CHUNK_SIZE_KEY, 128); + Path hfilePath = + new Path(TEST_UTIL.getDataTestDir(), "testMidKeyReadSuccessfullyFromRootIndexBlock"); + Compression.Algorithm compressAlgo = Compression.Algorithm.NONE; + int entryCount = 50000; + HFileContext context = new HFileContextBuilder().withBlockSize(4096).withIncludesTags(false) + .withDataBlockEncoding(DataBlockEncoding.NONE).withCompression(compressAlgo).build(); + + try (HFile.Writer writer = new HFile.WriterFactory(conf, new CacheConfig(conf)) + .withPath(fs, hfilePath).withFileContext(context).create()) { + + List keyValues = new ArrayList<>(entryCount); + for (int i = 0; i < entryCount; ++i) { + byte[] keyBytes = RandomKeyValueUtil.randomOrderedKey(RNG, i); + // A random-length random value. + byte[] valueBytes = RandomKeyValueUtil.randomValue(RNG); + KeyValue keyValue = + new KeyValue(keyBytes, null, null, HConstants.LATEST_TIMESTAMP, valueBytes); + writer.append(keyValue); + keyValues.add(keyValue); + } + } + + try (FSDataInputStream fsdis = fs.open(hfilePath)) { + long fileSize = fs.getFileStatus(hfilePath).getLen(); + FixedFileTrailer trailer = FixedFileTrailer.readFromStream(fsdis, fileSize); + + assertEquals(3, trailer.getMajorVersion()); + assertEquals(entryCount, trailer.getEntryCount()); + HFileContext meta = new HFileContextBuilder().withCompression(compressAlgo) + .withIncludesMvcc(false).withIncludesTags(false) + .withDataBlockEncoding(DataBlockEncoding.NONE).withHBaseCheckSum(true).build(); + ReaderContext readerContext = + new ReaderContextBuilder().withInputStreamWrapper(new FSDataInputStreamWrapper(fsdis)) + .withFilePath(hfilePath).withFileSystem(fs).withFileSize(fileSize).build(); + HFileBlock.FSReader blockReader = + new HFileBlock.FSReaderImpl(readerContext, meta, ByteBuffAllocator.HEAP, conf); + + MyEncoder encoder = new MyEncoder(); + HFileBlockIndex.CellBasedKeyBlockIndexReaderV2 dataBlockIndexReader = + new HFileBlockIndex.CellBasedKeyBlockIndexReaderV2(trailer.createComparator(), + trailer.getNumDataIndexLevels(), encoder); + + HFileBlock.BlockIterator blockIter = blockReader.blockRange(trailer.getLoadOnOpenDataOffset(), + fileSize - trailer.getTrailerSize()); + // Data index. We also read statistics about the block index written after + // the root level. + dataBlockIndexReader.readMultiLevelIndexRoot( + blockIter.nextBlockWithBlockType(BlockType.ROOT_INDEX), trailer.getDataIndexCount()); + NoOpEncodedSeeker noOpEncodedSeeker = (NoOpEncodedSeeker) encoder.encoderSeeker; + // Assert we have read midkey metadata successfully. + assertTrue(noOpEncodedSeeker.midLeafBlockOffset >= 0); + assertTrue(noOpEncodedSeeker.midLeafBlockOnDiskSize > 0); + assertTrue(noOpEncodedSeeker.midKeyEntry >= 0); + } + } + + static class MyEncoder implements HFileIndexBlockEncoder { + + EncodedSeeker encoderSeeker; + + @Override + public void saveMetadata(Writer writer) throws IOException { + NoOpIndexBlockEncoder.INSTANCE.saveMetadata(writer); + + } + + @Override + public void encode(BlockIndexChunk blockIndexChunk, boolean rootIndexBlock, DataOutput out) + throws IOException { + NoOpIndexBlockEncoder.INSTANCE.encode(blockIndexChunk, rootIndexBlock, out); + } + + @Override + public IndexBlockEncoding getIndexBlockEncoding() { + return NoOpIndexBlockEncoder.INSTANCE.getIndexBlockEncoding(); + } + + @Override + public EncodedSeeker createSeeker() { + encoderSeeker = NoOpIndexBlockEncoder.INSTANCE.createSeeker(); + return encoderSeeker; + } + + } }