wchevreuil commented on code in PR #6250:
URL: https://github.com/apache/hbase/pull/6250#discussion_r1766604655
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -1626,52 +1627,33 @@ private void parsePB(BucketCacheProtos.BucketCacheEntry
proto) throws IOExceptio
}
private void persistChunkedBackingMap(FileOutputStream fos) throws
IOException {
- 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);
+ backingMap.size(), fullyCachedFiles.size(), persistenceChunkSize);
Review Comment:
Please fix the log message. You have four `{}`, but are passing only three
params.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -390,17 +390,17 @@ private void startPersistenceRetriever(int[] bucketSizes,
long capacity) {
try {
retrieveFromFile(bucketSizes);
LOG.info("Persistent bucket cache recovery from {} is complete.",
persistencePath);
- } catch (IOException ioex) {
- LOG.error("Can't restore from file[{}] because of ", persistencePath,
ioex);
+ } catch (Throwable ex) {
+ LOG.error("Can't restore from file[{}] because of ", persistencePath,
ex);
Review Comment:
nit: since we are handling the error, shouldn't this be a warn? And let's
explain the cache will be reset and reload would happen.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketProtoUtils.java:
##########
@@ -62,31 +61,29 @@ static BucketCacheProtos.BucketCacheEntry toPB(BucketCache
cache,
.build();
}
- public static void serializeAsPB(BucketCache cache, FileOutputStream fos,
long chunkSize,
- long numChunks) throws IOException {
+ public static void serializeAsPB(BucketCache cache, FileOutputStream fos,
long chunkSize)
+ throws IOException {
+ // Write the new version of magic number.
+ fos.write(PB_MAGIC_V2);
+
int blockCount = 0;
- int chunkCount = 0;
int backingMapSize = cache.backingMap.size();
BucketCacheProtos.BackingMap.Builder builder =
BucketCacheProtos.BackingMap.newBuilder();
-
- fos.write(PB_MAGIC_V2);
- fos.write(Bytes.toBytes(chunkSize));
- fos.write(Bytes.toBytes(numChunks));
-
+ boolean firstChunkPersisted = false;
BucketCacheProtos.BackingMapEntry.Builder entryBuilder =
BucketCacheProtos.BackingMapEntry.newBuilder();
+
for (Map.Entry<BlockCacheKey, BucketEntry> entry :
cache.backingMap.entrySet()) {
blockCount++;
entryBuilder.clear();
entryBuilder.setKey(BucketProtoUtils.toPB(entry.getKey()));
entryBuilder.setValue(BucketProtoUtils.toPB(entry.getValue()));
builder.addEntry(entryBuilder.build());
-
if (blockCount % chunkSize == 0 || (blockCount == backingMapSize)) {
- chunkCount++;
- if (chunkCount == 1) {
+ if (firstChunkPersisted == false) {
// Persist all details along with the first chunk into
BucketCacheEntry
BucketProtoUtils.toPB(cache, builder.build()).writeDelimitedTo(fos);
+ firstChunkPersisted = true;
} else {
// Directly persist subsequent backing-map chunks.
builder.build().writeDelimitedTo(fos);
Review Comment:
maybe for consistency and simplicity, we should just write the
BucketCacheProtos.BucketCacheEntry before the for loop, then the backmap chunks
only within this loop. That way we wouldn't need this `firstChunkPersisted`
thread.
--
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]
