Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-27232 Fix checking for encoded block size when deciding if bloc… #4640

Merged
merged 6 commits into from
Jul 25, 2022
Merged

HBASE-27232 Fix checking for encoded block size when deciding if bloc… #4640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05ba4b4
HBASE-27232 Fix checking for encoded block size when deciding if bloc…
wchevreuil Jul 21, 2022
79fd61e
fixing comment
wchevreuil Jul 21, 2022
4bf5d49
addressing review comments & spotless checks
wchevreuil Jul 21, 2022
1b46055
fixing UT error and checkstyle
wchevreuil Jul 22, 2022
8eb91cc
spotless
wchevreuil Jul 22, 2022
3b57fe6
review changes
wchevreuil Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ public int getOnDiskSizeWithHeader() {
}

/** Returns the on-disk size of the data part + checksum (header excluded). */
int getOnDiskSizeWithoutHeader() {
public int getOnDiskSizeWithoutHeader() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's feasible, but if you move the test to TestHFile, for instance, you don't need to make this method public.

Apache9 marked this conversation as resolved.
Show resolved Hide resolved
return onDiskSizeWithoutHeader;
}

Expand Down
21 changes: 13 additions & 8 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,10 @@ public HFileWriterImpl(final Configuration conf, CacheConfig cacheConf, Path pat
}
closeOutputStream = path != null;
this.cacheConf = cacheConf;
float encodeBlockSizeRatio = conf.getFloat(UNIFIED_ENCODED_BLOCKSIZE_RATIO, 1f);
this.encodedBlockSizeLimit = (int) (hFileContext.getBlocksize() * encodeBlockSizeRatio);
float encodeBlockSizeRatio = conf.getFloat(UNIFIED_ENCODED_BLOCKSIZE_RATIO, 0f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this changes the default behavior i believe. do you think this change is applicable to most users (i.e. net positive for them)? not against it, just asking... alternatively we can add handling of 0 value below and leave default. i have no opinion since i dont fully grasp the feature yet.

Copy link
Contributor Author

@wchevreuil wchevreuil Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't change the default behaviour in the sense that if "hbase.writer.unified.encoded.blocksize.ratio" isn't set, we consider only the unencoded size for calculating block limit, which is the same with previous if condition on checkBlockBoundary method.

The difference is when "hbase.writer.unified.encoded.blocksize.ratio" is set, as we now can have 64KB of encoded data (whilst it would never be possible before).

this.encodedBlockSizeLimit = encodeBlockSizeRatio >0 ?
wchevreuil marked this conversation as resolved.
Show resolved Hide resolved
(int) (hFileContext.getBlocksize() * encodeBlockSizeRatio) : 0;
wchevreuil marked this conversation as resolved.
Show resolved Hide resolved

finishInit(conf);
if (LOG.isTraceEnabled()) {
LOG.trace("Writer" + (path != null ? " for " + path : "") + " initialized with cacheConf: "
Expand Down Expand Up @@ -309,12 +311,15 @@ protected void finishInit(final Configuration conf) {
* At a block boundary, write all the inline blocks and opens new block.
*/
protected void checkBlockBoundary() throws IOException {
// For encoder like prefixTree, encoded size is not available, so we have to compare both
// encoded size and unencoded size to blocksize limit.
if (
blockWriter.encodedBlockSizeWritten() >= encodedBlockSizeLimit
|| blockWriter.blockSizeWritten() >= hFileContext.getBlocksize()
) {
boolean shouldFinishBlock = false;
//This means hbase.writer.unified.encoded.blocksize.ratio was set to something different from 0
Copy link
Contributor

@bbeaudreault bbeaudreault Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any implication on the comment that was here before which says we need to compare both? granted I think prefixTree has been removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we don't have prefix tree anymore. So with the previous "||" condition, we could still not respect the encoded size desired if the data shrinkage by encoding is higher than the configured "hbase.writer.unified.encoded.blocksize.ratio' value. This change also allows for defining a 1:1 ration where we would then use the encoded size for the block limit.

//and we should use the encoding ratio
if (encodedBlockSizeLimit > 0){
shouldFinishBlock = blockWriter.encodedBlockSizeWritten() >= encodedBlockSizeLimit;
} else {
shouldFinishBlock = blockWriter.blockSizeWritten() >= hFileContext.getBlocksize();
}
if(shouldFinishBlock) {
finishBlock();
writeInlineBlocks(false);
newBlock();
Expand Down
63 changes: 63 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStoreFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@
import org.apache.hadoop.hbase.io.hfile.BlockCacheFactory;
import org.apache.hadoop.hbase.io.hfile.CacheConfig;
import org.apache.hadoop.hbase.io.hfile.CacheStats;
import org.apache.hadoop.hbase.io.hfile.FixedFileTrailer;
import org.apache.hadoop.hbase.io.hfile.HFile;
import org.apache.hadoop.hbase.io.hfile.HFileBlock;
import org.apache.hadoop.hbase.io.hfile.HFileContext;
import org.apache.hadoop.hbase.io.hfile.HFileContextBuilder;
import org.apache.hadoop.hbase.io.hfile.HFileDataBlockEncoder;
Expand Down Expand Up @@ -1141,4 +1144,64 @@ public void testDataBlockEncodingMetaData() throws IOException {
byte[] value = fileInfo.get(HFileDataBlockEncoder.DATA_BLOCK_ENCODING);
assertArrayEquals(dataBlockEncoderAlgo.getNameInBytes(), value);
}

@Test
public void testDataBlockSizeEncoded() throws Exception {
// Make up a directory hierarchy that has a regiondir ("7e0102") and familyname.
Path dir = new Path(new Path(this.testDir, "7e0102"), "familyname");
Path path = new Path(dir, "1234567890");

DataBlockEncoding dataBlockEncoderAlgo =
DataBlockEncoding.FAST_DIFF;

conf.setDouble("hbase.writer.unified.encoded.blocksize.ratio", 1);

cacheConf = new CacheConfig(conf);
HFileContext meta = new HFileContextBuilder().withBlockSize(BLOCKSIZE_SMALL)
.withChecksumType(CKTYPE)
.withBytesPerCheckSum(CKBYTES)
.withDataBlockEncoding(dataBlockEncoderAlgo)
.build();
// Make a store file and write data to it.
StoreFileWriter writer = new StoreFileWriter.Builder(conf, cacheConf, this.fs)
.withFilePath(path)
.withMaxKeyCount(2000)
.withFileContext(meta)
.build();
writeStoreFile(writer);
//writer.close();
wchevreuil marked this conversation as resolved.
Show resolved Hide resolved

HStoreFile storeFile = new HStoreFile(fs, writer.getPath(), conf,
cacheConf, BloomType.NONE, true);
storeFile.initReader();
StoreFileReader reader = storeFile.getReader();

Map<byte[], byte[]> fileInfo = reader.loadFileInfo();
byte[] value = fileInfo.get(HFileDataBlockEncoder.DATA_BLOCK_ENCODING);
assertEquals(dataBlockEncoderAlgo.name(), Bytes.toString(value));

HFile.Reader fReader = HFile.createReader(fs, writer.getPath(), storeFile.getCacheConf(),
true, conf);

FSDataInputStreamWrapper fsdis = new FSDataInputStreamWrapper(fs, writer.getPath());
long fileSize = fs.getFileStatus(writer.getPath()).getLen();
FixedFileTrailer trailer =
FixedFileTrailer.readFromStream(fsdis.getStream(false), fileSize);
long offset = trailer.getFirstDataBlockOffset(),
max = trailer.getLastDataBlockOffset();
HFileBlock block;
int blockCount = 0;
while (offset <= max) {
block = fReader.readBlock(offset, -1, /* cacheBlock */
false, /* pread */ false,
/* isCompaction */ false, /* updateCacheMetrics */
false, null, null);
offset += block.getOnDiskSizeWithHeader();
blockCount += 1;
double diff = block.getOnDiskSizeWithHeader() - BLOCKSIZE_SMALL;
if(offset <= max)
assertTrue(diff >=0 && diff < (BLOCKSIZE_SMALL*0.05));
}
}

}