wchevreuil commented on code in PR #6183:
URL: https://github.com/apache/hbase/pull/6183#discussion_r1735980762
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1358,8 +1397,14 @@ void persistToFile() throws IOException {
}
File tempPersistencePath = new File(persistencePath +
EnvironmentEdgeManager.currentTime());
try (FileOutputStream fos = new FileOutputStream(tempPersistencePath,
false)) {
- fos.write(ProtobufMagic.PB_MAGIC);
- BucketProtoUtils.toPB(this).writeDelimitedTo(fos);
+ if (!enableChunkedPersistence) {
+ LOG.debug("Persist in old persistence format.");
+ fos.write(ProtobufMagic.PB_MAGIC);
+ BucketProtoUtils.toPB(this).writeDelimitedTo(fos);
+ } else {
+ LOG.debug("Persist in new chunked persistence format.");
+ persistChunkedBackingMap(fos);
+ }
Review Comment:
Why are we making this configurable when we know the _old way_ won't work
for larger caches?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1511,6 +1629,71 @@ private void parsePB(BucketCacheProtos.BucketCacheEntry
proto) throws IOExceptio
verifyCapacityAndClasses(proto.getCacheCapacity(), proto.getIoClass(),
proto.getMapClass());
}
+ private void persistChunkedBackingMap(FileOutputStream fos) throws
IOException {
+ fos.write(PB_MAGIC_V2);
+ int backingMapSize = backingMap.size();
+ long numChunks = backingMap.size() / persistenceChunkSize;
+ if (backingMap.size() % persistenceChunkSize != 0) {
+ numChunks += 1;
+ }
+
+ LOG.debug("persistToFile: before persisting backing map size: {}, "
+ + "fullycachedFiles size: {}, chunkSize: {}, numberofChunks: {}",
+ backingMap.size(), fullyCachedFiles.size(), persistenceChunkSize,
numChunks);
+
+ fos.write(convertToBytes(persistenceChunkSize));
+ fos.write(convertToBytes(numChunks));
+
+ int blockCount = 0;
+ int chunkCount = 0;
+
+ BucketCacheProtos.BackingMap.Builder builder =
BucketCacheProtos.BackingMap.newBuilder();
Review Comment:
Why are we creating proto builder here? Can't this go to BucketProtoUtils?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1345,6 +1366,24 @@ static List<RAMQueueEntry>
getRAMQueueEntries(BlockingQueue<RAMQueueEntry> q,
return receptacle;
}
+ byte[] convertToBytes (long value) {
+ byte[] bytes = new byte[Long.BYTES];
+ int length = bytes.length;
+ for (int i = 0; i < length; i++) {
+ bytes[length - i - 1] = (byte) (value & 0xFF);
+ value >>= 8;
+ }
+ return bytes;
+ }
+
+ long convertToLong(byte[] bytes) {
+ long value = 0;
+ for (byte b : bytes) {
+ value = (value << 8) + (b & 0xFF);
+ }
+ return value;
+ }
+
Review Comment:
There are already `Bytes.toBytes`, `Bytes.toLong` for this.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1457,6 +1509,72 @@ private void verifyCapacityAndClasses(long capacitySize,
String ioclass, String
}
}
+ private void parsePB(BucketCacheProtos.BucketCacheEntry firstChunk,
Review Comment:
Most of this is duplicating code from former
`parsePB(BucketCacheProtos.BucketCacheEntry proto)`. Can't we move the common
pieces to be reused by both methods?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
