HBASE-28839: Handle all types of exceptions during retrieval of bucket-cache from persistence. #6250
Conversation
This comment has been minimized.
This comment has been minimized.
jhungund
force-pushed
the
HBASE-28839
branch
from
43cf0ad to
daa93dc
Compare
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
We should rather understand and solve the trigger, than put this bandaid here.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
jhungund
force-pushed
the
HBASE-28839
branch
from
daa93dc to
f528fae
Compare
jhungund
force-pushed
the
HBASE-28839
branch
from
f528fae to
7cae31e
Compare
This comment has been minimized.
This comment has been minimized.
…t-cache from persistence. In certain scenarios, where there is discrepancy between the number of chunks persisted in the file and the number of chunks stored in the persistence file. This is because the bucket cache may be operated upon in parallel. During the retrievel of bucket-cache from persistence, it was observed that, if an exception, other than the IOException occurs, the exception is not logged and also the retrieval thread exits leaving the bucket cache in an uninitialised state, leaving it unusable. With this change, the retrieval code does not rely on the metadata information (number of chunks) and instead, it reads from the file-stream as long as the data is available to be read. This change, enables the retrieval thread to print all types of exceptions and also reinitialises the bucket cache and makes it reusable. Change-Id: I81b7f5fe06945702bbc59df96d054f95f03de499
jhungund
force-pushed
the
HBASE-28839
branch
from
7cae31e to
c5275f0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } 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); |
There was a problem hiding this 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/BucketCache.java
Show resolved
Hide resolved
| 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); |
There was a problem hiding this 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 comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Change-Id: Icf6cdcf829e7d4bd16f50f48fd02059b415f2d09
This comment has been minimized.
This comment has been minimized.
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Needs to also address the spotless issues.
| // Create the first chunk and persist all details along with it. | ||
| while (entrySetIter.hasNext()) { | ||
| blockCount++; | ||
| Map.Entry<BlockCacheKey, BucketEntry> entry = entrySetIter.next(); | ||
| addToBuilder(entry, entryBuilder, builder); | ||
| if (blockCount % chunkSize == 0 || (blockCount == backingMapSize)) { | ||
| chunkCount++; | ||
| if (chunkCount == 1) { | ||
| // Persist all details along with the first chunk into BucketCacheEntry | ||
| BucketProtoUtils.toPB(cache, builder.build()).writeDelimitedTo(fos); | ||
| } else { | ||
| // Directly persist subsequent backing-map chunks. | ||
| break; | ||
| } | ||
| } | ||
| builder.clear(); |
There was a problem hiding this comment.
Why do we need two while loops? just do the BucketProtoUtils.toPB(cache, builder.build()).writeDelimitedTo(fos); before the loop. That would write all the BucketCacheEntry meta info at the beginning of the file with an empty backingMap at this point, but that's fine as we'll write all backingMap chunks subsequently and this code would be cleaner, I suppose.
| BucketCacheProtos.BucketCacheEntry firstChunk = | ||
| BucketCacheProtos.BucketCacheEntry.parseDelimitedFrom(in); | ||
| parseFirstChunk(firstChunk); | ||
|
|
||
| // Subsequent chunks have the backingMap entries. | ||
| for (int i = 1; i < numChunks; i++) { | ||
| LOG.info("Reading chunk no: {}", i + 1); | ||
| int numChunks = 0; | ||
| while (in.available() > 0) { | ||
| parseChunkPB(BucketCacheProtos.BackingMap.parseDelimitedFrom(in), | ||
| firstChunk.getDeserializersMap()); | ||
| LOG.info("Retrieved chunk: {}", i + 1); | ||
| numChunks++; | ||
| } |
There was a problem hiding this comment.
If we change the way we write to the file as I suggested, so that we have only the BucketCacheProtos.BucketCacheEntry at the beginning followed by all BucketCacheProtos.BackingMap chunks , then we should change the naming here, firstChunk should now be entry. And these parseFirstChunk, parseChunkPB are not really parsing anything (parsing is delegated to the proto utils), but rather updating the cache index structures, so we should rename it to something like updateCacheIndex.
Also looking at parseFirstChunck and parseChunk, we should replace duplicate code inside parseFirstChunck by call to parseChunk. Or since parseFirstChunk is only used here, we could just get rid of it and simply do:
fullyCachedFiles.clear(); fullyCachedFiles.putAll(BucketProtoUtils.fromPB(entry.getCachedFilesMap()));
Whilst the backingMap and blocksByHfile would get updated all in the loop.
jhungund
force-pushed
the
HBASE-28839
branch
from
533b590 to
58ebef3
Compare
Change-Id: I97936a683673ff89e04a15bc66542fb93a32fe8a
jhungund
force-pushed
the
HBASE-28839
branch
from
58ebef3 to
409554d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @wchevreuil, the failing test seems to have passed in the the rerun. I have addressed your review comments. Please take a look. |
| .putAllDeserializers(CacheableDeserializerIdManager.save()) | ||
| .putAllCachedFiles(toCachedPB(cache.fullyCachedFiles)).setBackingMap(backingMap) | ||
| .putAllCachedFiles(toCachedPB(cache.fullyCachedFiles)) | ||
| .setBackingMap(backingMapBuilder.build()) |
There was a problem hiding this comment.
We need to pass an empty backingmap here otherwise we see an exception of an incomplete object. Hence, we just pass an empty backing map along with the metadata. Subsequently, we persist all entries of the backing map in chunks.
There was a problem hiding this comment.
We don't need a builder, just pass an empty map. Or, since we don't persist any map entries within the BucketCacheEntry proto object, just remove the map from the protobuf message. We already changed the persistent file format on HBASE-28805, as long as this can land on all related branches whilst HBASE-28805 has not made into any release, we are free to change the format.
There was a problem hiding this comment.
Hi @wchevreuil, we will need to retain the old format of protobuf message to maintain the backward compatibility. Hence, the we cannot change the protobuf message. We can reuse this protobuf message by persisting an empty backing map instead of introducing a new version.
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
Show resolved
Hide resolved
| .putAllDeserializers(CacheableDeserializerIdManager.save()) | ||
| .putAllCachedFiles(toCachedPB(cache.fullyCachedFiles)).setBackingMap(backingMap) | ||
| .putAllCachedFiles(toCachedPB(cache.fullyCachedFiles)) | ||
| .setBackingMap(backingMapBuilder.build()) |
There was a problem hiding this comment.
We don't need a builder, just pass an empty map. Or, since we don't persist any map entries within the BucketCacheEntry proto object, just remove the map from the protobuf message. We already changed the persistent file format on HBASE-28805, as long as this can land on all related branches whilst HBASE-28805 has not made into any release, we are free to change the format.
…t-cache from persistence. (#6250) Signed-off-by: Wellington Chevreuil <[email protected]>
…t-cache from persistence. (apache#6250) Signed-off-by: Wellington Chevreuil <[email protected]>
…t-cache from persistence. (#6250) (#6288) Signed-off-by: Wellington Chevreuil <[email protected]>
HBASE-28839: Handle all types of exceptions during retrieval of bucket-cache from persistence. (apache#6250) Signed-off-by: Wellington Chevreuil <[email protected]> Change-Id: I1e8147bffcc456a59375ec67471e736079e5e107 (cherry picked from commit 2e0b01f)
HBASE-28839: Handle all types of exceptions during retrieval of bucket-cache from persistence. (apache#6250) Signed-off-by: Wellington Chevreuil <[email protected]> Change-Id: I1e8147bffcc456a59375ec67471e736079e5e107 (cherry picked from commit 2e0b01f)
…t-cache from persistence. (apache#6250) (apache#6288) Signed-off-by: Wellington Chevreuil <[email protected]> Change-Id: Ied978410cc7d353e675144b877365465fcf96c67
3 participants
During the retrievel of bucket-cache from persistence, it was observed that, if an exception, other than the IOException occurs, the exception is not logged and also the retrieval thread exits leaving the bucket cache in an uninitialised state, leaving it unusable.
This change, enables the retrieval thread to peint all types of exceptions and also reinitialises the bucket cache and makes it reusable.
Unfortunately, the exception was seen due to a parallel execution of eviction during the execution persistence of backing map. Hence, this use-case may not be tested via a unit test.
Change-Id: I81b7f5fe06945702bbc59df96d054f95f03de499